-
Notifications
You must be signed in to change notification settings - Fork 7
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
adds a CompositeSpanExporter #376
adds a CompositeSpanExporter #376
Conversation
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite. Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
I have considered moving the |
|
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.
Looks fine for the demo, but ideally we can get rid of our own composite. Creating it before we start the SDK would mean that we never have a gap between when the SDK is created and when the customer adds the exporter.
import io.opentelemetry.sdk.trace.data.SpanData | ||
import io.opentelemetry.sdk.trace.export.SpanExporter | ||
|
||
public class CompositeSpanExporter : SpanExporter { |
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.
This could be internal
OpenTelemetrySdk( | ||
openTelemetryClock = openTelemetryClock, | ||
spanProcessor = EmbraceSpanProcessor(EmbraceSpanExporter(spansSink)) |
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.
Hmm. The reason you can't use the default composite one is because we are initializing this too early, before the app can add the exporter instance? If this were happening later, I can imagine a list of exporters passed into InitModuleImpl
and we just call SpanExporter.composite()
here
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.
Exactly. We are exporting the emb-startup-moment
span, though.
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 pending resolution of Hanson's comments
e10bbb5
to
3d88cba
Compare
8ddce28
to
ea6484a
Compare
3d88cba
to
18d92fd
Compare
ea6484a
to
89682f7
Compare
Goal
Creates a CompositeSpanExporter to add new instances of SpanExporter at runtime.
I considered using the
io.opentelemetry.SpanExporter.composite()
method (link here). Still, it can't be modified at runtime, so it's not helpful to the purpose of allowing a client app to add a SpanExporter.