Skip to content
This repository was archived by the owner on Aug 28, 2020. It is now read-only.

Adding metrics exporter #14

Merged
merged 38 commits into from
May 1, 2020
Merged

Adding metrics exporter #14

merged 38 commits into from
May 1, 2020

Conversation

codeboten
Copy link
Contributor

@codeboten codeboten commented Apr 29, 2020

Initial implementation of a metrics exporter. As part of the changes here, I'm refactoring the SpanExporter to use protos directly, which removes the dependency on the lightstep-tracer ibrary as well as thrift.

There are a few parts in this change:

  • adding a MetricExporter for Lightstep
  • adding protobuf code
  • cleaning up some pylint/flake

Note:
The collector_pb2.py and metrics_pb2.py files are generated, probably not worth reviewing.

Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comments for a few things that could be checked out first.

Comment on lines +63 to +64
start = start or 0
end = end or 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the idea behind this or 0? Can start or end be False or something similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They can be None


def shutdown(self) -> None:
self.tracer.flush()
"""Flush remaining spans"""
# self.tracer.flush()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the docstring, removed the comment. Currently there's no need to implement anything in the shutdown.



class LightStepSpanExporter(LightstepSpanExporter):
"""Backwards compatibility wrapper class"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a DeprecationWarning here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!


def _generate_guid():
"""Construct a guid - a random 64 bit integer"""
return _GUID_RNG.getrandbits(64) - 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a heads up, this could return a negative value (because of the - 1)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Fixed.

Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@codeboten codeboten merged commit e673826 into master May 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants