Skip to content
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

Send to multiple exporters from tracer #397

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

c24t
Copy link
Member

@c24t c24t commented Nov 14, 2018

This is an experimental branch to address #181. It changes Tracer such that it can use multiple exporters at once.

Copy link

@zoidyzoidzoid zoidyzoidzoid left a comment

Choose a reason for hiding this comment

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

If this is something we wanna support and not offload to opencensus-service, then the changes look good to me, other than the nitpicks.

:class:`.Loggingexporter`, :class:`.Zipkinexporter`,
:class:`.GoogleCloudexporter`
"""
:type exporters: class:`~opencensus.trace.exporters.base.exporter`

Choose a reason for hiding this comment

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

This should be type exporter and param exporter still, right? Judging from the contents.

Copy link
Member Author

Choose a reason for hiding this comment

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

It should! good catch.

@@ -120,7 +120,8 @@ def end_span(self, *args, **kwargs):
with self._spans_list_condition:
if cur_span in self._spans_list:
span_datas = self.get_span_datas(cur_span)
self.exporter.export(span_datas)
for exporter in self.exporters:
exporter.export(span_datas)

Choose a reason for hiding this comment

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

If someone tries to send something to multiple exporters, but one is incorrectly configured, we should try export to the others still, right, are we just gonna hope that each Exporter.emit swallows the exceptions and doesn't raise any?

Copy link
Contributor

Choose a reason for hiding this comment

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

I remember we had discussion about supporting multiple exporters and propagators, and people in that meeting generally agreed not to expose the interface for multiple exporters/propagators, but let people implement something like AggregatedExporter/Propagator. The idea is that you can implement a wrapper exporter that calls into multiple exporters, and you can decide what's the expected behavior - either abort the others if one failed, or continue.

Can we have a clarification here - do we want to have multiple exports/propagators interface for all languages? Or we want to stick with the aggregated approach?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@reyang both Java and Go libraries support multiple destinations today. From a testing perspective or migration perspective this is a good feature to support. Of course, long term this could be handed off to the agent/collector, but requiring the agent/collector limits flexibility. I would propose supporting multiple destinations.

Copy link
Contributor

Choose a reason for hiding this comment

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

@flands thank you! What about multiple propagators?

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO this is less of a concern -- switching and testing is straightforward and migration path is cutover. Let me check OC service team and get back shortly.

Copy link
Member Author

Choose a reason for hiding this comment

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

@reyang I wasn't around for the discussion, can you link me? I don't think this is necessarily the best design, and I'm willing to believe we should add AggregatedExporter/Propagator classes instead.

@flands IMO if the API were closer to the java client's I think it would make sense to argue for a particular implementation, but as it is I think we have some flexibility here, and don't necessarily need to change the tracer.

Copy link
Contributor

Choose a reason for hiding this comment

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

@c24t I couldn't find the discussion, it was Jul. or Aug. this year.

I remember at that time we were talking about how flush API should be provided, whether the user would need to call flush for each individual exporter (if we support multi), or a top level flush at tracer level should be provided (and should we block the flush API until all exporters have finished flush, what do we do if one exporter failed).

Copy link
Member Author

Choose a reason for hiding this comment

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

Want to take the conversation around the design to #181?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep! #181 seems to be a good place for discussion.

@flands
Copy link
Collaborator

flands commented Nov 15, 2018

@c24t this appears to work, thanks! note, there are other parts of the library that also need to be updated. For example, opencensus/trace/ext/flask/flask_middleware.py

@ocervell
Copy link
Contributor

@c24t looks cool. Does that mean we'll have one background thread / exporter here ?

@c24t
Copy link
Member Author

c24t commented Nov 16, 2018

Does that mean we'll have one background thread / exporter here ?

We will for each exporter that's using a background thread transporter, but we're blocking between calls to export. If we expect users to use many exporters with blocking transports at once we might want to make the calls from (e.g.) an async executor instead.

@c24t c24t requested review from aabmass, hectorhdzg, lzchen, songy23 and a team as code owners May 13, 2021 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants