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

Pipeline builder for stdout trace exporter #224

Merged
merged 2 commits into from
Sep 24, 2020

Conversation

frigus02
Copy link
Member

@frigus02 frigus02 commented Sep 22, 2020

Part of #156

Implement the pipeline builder API for the stdout trace exporter, similar to the other exporters. With this a stdout trace pipeline can now be installed like this:

let tracer = opentelemetry::exporter::trace::stdout::new_pipeline().install();

I made 2 small changes along the way, which seemed to make sense to me. Happy to change in either direction, though. These are:

  • Defer requirement of all additional writer traits to the PipelineBuilder::install function. Before Debug was required for the builder and Send + 'static were required only for the exporter. Now the builder is happy with only Write and requires Debug + Send + 'static for installation. I think I don't see any downsides here. We could also require all of these traits for the builder already. Not sure what's better.

  • Defer wrapping writer in Mutex to when the exporter is created, which seems to be the point at which it's needed. Does this have any downsides?

Additional note:

This makes the pipeline the only way to construct a stdout exporter. This is in line with other exporters, but I'm not sure it's desired. Should we make Exporter::new public, so users can still create an exporter and setup a pipeline manually?

Update: Exporter::new is now public.

Copy link
Member

@jtescher jtescher left a comment

Choose a reason for hiding this comment

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

This looks good @frigus02, I think making Exporter::new public is probably a good idea, can do that here or in a follow up PR

@frigus02
Copy link
Member Author

Sounds good. I just pushed a commit to make Exporter::new public and add docs for it.

@jtescher jtescher merged commit 33ee164 into open-telemetry:master Sep 24, 2020
@frigus02 frigus02 deleted the stdout-pipeline branch September 24, 2020 16:11
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.

2 participants