-
Notifications
You must be signed in to change notification settings - Fork 250
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
Fix span batching to stackdriver #425
Fix span batching to stackdriver #425
Conversation
@kornholi is this fixing a regression in the stackdriver exporter or an improvement? |
Regression from #291 |
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.
LGTM, please file a ticket for the stackdriver quota issue (instead of the FIXME comment) if this is still causing problems for you.
# Write spans to Stackdriver | ||
for _, sds in trace_span_map.items(): | ||
# convert to the legacy trace json for easier refactoring | ||
# TODO: refactor this to use the span data directly | ||
trace = span_data.format_legacy_trace_json(sds) | ||
stackdriver_spans = self.translate_to_stackdriver(trace) | ||
self.client.batch_write_spans(project, stackdriver_spans) | ||
stackdriver_spans.extend(self.translate_to_stackdriver(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.
Out of curiosity: why change this to a generator if you're just appending the results?
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 avoid intermediate lists
* Fix span batching to stackdriver * Lose FIXME comment
Currently every trace is being exported individually, burning through Stackdriver quota.
This change was deployed at 5:46 PM.
@c24t @liyanhui1228 could you take a look?