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

Switch to async exporters #232

Merged
merged 7 commits into from
Oct 1, 2020
Merged

Switch to async exporters #232

merged 7 commits into from
Oct 1, 2020

Conversation

jtescher
Copy link
Member

@jtescher jtescher commented Sep 27, 2020

This change updates the `SpanExporter interface to export asynchronously. The primary API change is:

- fn export(&self, batch: Vec<Arc<trace::SpanData>>) -> trace::ExportResult;
+ async fn export(&self, batch: &[Arc<trace::SpanData>]) -> trace::ExportResult;

Implementation notes:

  • Switch from Vec<_> to &[_] in exporters to avoid additional allocation when batch exporting.
  • Currently uses async_trait, could change method signature to avoid the dependency if preferable.
  • BatchSpanProcessorWorker's Future impl now awaits sub-futures so replaced with async block for greater clarity.
  • The thrift and grpcio crates are not currently async, so some work still necessary to make jaeger and otlp exporters async (they currently call blocking code in a future, which is not great)

@jtescher jtescher requested a review from a team as a code owner September 27, 2020 22:32
Copy link
Contributor

@TommyCpp TommyCpp left a comment

Choose a reason for hiding this comment

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

Nice job! I think grpcio generally support async(I could be wrong since I didn't use the async part before). But didn't find thrift async crate. May need to figure out how to make that work.

opentelemetry-otlp/src/span.rs Show resolved Hide resolved
@jtescher
Copy link
Member Author

@TommyCpp yeah may have to just use the thrift crate to write to a buffer synchronously and then send async

Copy link
Contributor

@TommyCpp TommyCpp left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jtescher
Copy link
Member Author

jtescher commented Sep 29, 2020

Ok going to see if making the jaeger exporter async is a quick change, if not can do in a follow up PR

Copy link
Member

@frigus02 frigus02 left a comment

Choose a reason for hiding this comment

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

Looks really nice 👍

src/sdk/trace/span_processor.rs Show resolved Hide resolved
src/sdk/trace/span_processor.rs Show resolved Hide resolved
@jtescher
Copy link
Member Author

@frigus02 good catch, still toying with the jaeger exporter but will fix those as well 🙏

@jtescher
Copy link
Member Author

jtescher commented Oct 1, 2020

Ok switched jaeger collector client to use the isahc http client as it is executor agnostic, and manually maintain a buffer for the UDP agent and flush async. Could consider making the zipkin exporter executor agnostic in a future PR, but at least all the exporters are now non-blocking 🎉 .

I also removed the max packet size option for the agent exporter as I don't think you can actually configure the packet size in any of the implementations.

Copy link
Member

@frigus02 frigus02 left a comment

Choose a reason for hiding this comment

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

I'm not familiar with the Jaeger exporter, but as far as I understand these changes look nice.

Is it worth documenting, that users might want to enable the tokio or async-std feature when they're using the Jaeger agent?

@jtescher jtescher merged commit 0979c06 into master Oct 1, 2020
@jtescher jtescher deleted the async-exporters branch October 1, 2020 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants