-
Notifications
You must be signed in to change notification settings - Fork 248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Batch exporting spans once a while #63
Batch exporting spans once a while #63
Conversation
|
||
def export(self, trace): | ||
def emit(self, trace): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add emit() to base.Exporter, and add doc comments explaining emit() and export()?
The naming is a little weird, since I don't usually think of "transport" for something that's just printing or appending to a file. But I don't have a better suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
|
||
class _Worker(object): | ||
def __init__(self, exporter, grace_period=_DEFAULT_GRACE_PERIOD, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The google-cloud-python code which inspired this is well documented, e.g. https://github.com/GoogleCloudPlatform/google-cloud-python/blob/master/logging/google/cloud/logging/handlers/transports/background_thread.py#L65 . Can you write/copy doc comments into this code to match?
It would be nicest if we could both use the same code, but I don't think that's feasible right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -62,12 +63,14 @@ def __init__( | |||
service_name='my_service', | |||
host_name=DEFAULT_HOST_NAME, | |||
port=DEFAULT_PORT, | |||
endpoint=DEFAULT_ENDPOINT): | |||
endpoint=DEFAULT_ENDPOINT, | |||
transport=sync.SyncTransport): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the doc comment above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
trace/tests/system/flask/main.py
Outdated
from opencensus.trace.exporters.transports import background_thread | ||
exporter = stackdriver_exporter.StackdriverExporter( | ||
transport=background_thread.BackgroundThreadTransport) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To check my understanding, the flask test uses async transport and the django test uses sync transport? I think this is a good idea, but we should add a comment here and in the django test explaining that they are intentionally different to test both code paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes correct. Comment added.
Closes #59
This PR is doing the following things: