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

Allow adding span processors when building a tracer provider. #2223

Merged

Conversation

anuraaga
Copy link
Contributor

@anuraaga anuraaga commented Dec 8, 2020

Similar to how we added to OpenTelemetrySdk.Builder I think we should have it here for symmetry and for users that only interact with the tracer-sdk.

@codecov
Copy link

codecov bot commented Dec 8, 2020

Codecov Report

Merging #2223 (108b948) into master (17fa4e9) will increase coverage by 87.49%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master    #2223       +/-   ##
=============================================
+ Coverage          0   87.49%   +87.49%     
- Complexity        0     2431     +2431     
=============================================
  Files             0      269      +269     
  Lines             0     8380     +8380     
  Branches          0      889      +889     
=============================================
+ Hits              0     7332     +7332     
- Misses            0      723      +723     
- Partials          0      325      +325     
Impacted Files Coverage Δ Complexity Δ
...in/java/io/opentelemetry/sdk/OpenTelemetrySdk.java 100.00% <100.00%> (ø) 10.00 <0.00> (?)
.../io/opentelemetry/sdk/trace/SdkTracerProvider.java 100.00% <100.00%> (ø) 14.00 <1.00> (?)
.../io/opentelemetry/sdk/trace/TracerSharedState.java 93.54% <100.00%> (ø) 10.00 <0.00> (?)
.../main/java/io/opentelemetry/api/trace/TraceId.java 96.55% <0.00%> (ø) 16.00% <0.00%> (?%)
...ava/io/opentelemetry/api/trace/PropagatedSpan.java 83.33% <0.00%> (ø) 18.00% <0.00%> (?%)
...pentelemetry/sdk/metrics/LongUpDownCounterSdk.java 100.00% <0.00%> (ø) 3.00% <0.00%> (?%)
.../sdk/trace/samplers/ParentBasedSamplerBuilder.java 100.00% <0.00%> (ø) 6.00% <0.00%> (?%)
...ava/io/opentelemetry/sdk/internal/SystemClock.java 100.00% <0.00%> (ø) 4.00% <0.00%> (?%)
...ry/sdk/metrics/AbstractAsynchronousInstrument.java 100.00% <0.00%> (ø) 2.00% <0.00%> (?%)
...ava/io/opentelemetry/sdk/trace/SdkSpanBuilder.java 100.00% <0.00%> (ø) 37.00% <0.00%> (?%)
... and 262 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17fa4e9...b1aebbb. Read the comment docs.

@@ -216,10 +216,6 @@ public OpenTelemetrySdk build() {
MeterProvider meterProvider = buildMeterProvider();
TracerSdkProvider tracerProvider = buildTracerProvider();

for (SpanProcessor spanProcessor : spanProcessors) {
tracerProvider.addSpanProcessor(spanProcessor);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

the downside about removing this is that now we won't be adding span processors to a pre-configured SDK. Not sure if we're ok with that or not. I know we've chatted about removing the ability to provide the fully configured SDK altogether, which would make this (obviously) just fine.

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

I think the reason to add this is wrong, we should remove the capability from the OpenTelemetry.Builder and add only this one.

@jkwatson
Copy link
Contributor

jkwatson commented Dec 8, 2020

I think the reason to add this is wrong, we should remove the capability from the OpenTelemetry.Builder and add only this one.

Can you explain why you say this? @iNikem has very much wanted this capability in the configuration of OpenTelemetrySdk itself. Providing the option in either place will let people either configure the whole public API, or just the tracing part of it, as they desire.

@bogdandrutu
Copy link
Member

Here are some reasons:

  1. Duplicate ways to achieve the same thing as well as duplicate code. I don't think it needs explanation why this is wrong, but a small example is anything changes in one of these APIs and you have two deprecated APIs instead of one, confusion for the users, etc.
  2. It is possible to configure the TracerProvider, so no need to have the global way.

We need to remember that larger our API surface is, more things we need to maintain and be careful about. So limiting the API is always the best decision. The trade-off is 1 extra line on the configuration code so not worth it.

@bogdandrutu
Copy link
Member

In #2231, I removed all duplicate ways. You can see in the test that I can still set all the options, and with not that much extra code.

Copy link
Contributor Author

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

@bogdandrutu I thought the PR description being simple would be easier to understand but guess not :)

The real motivation for this is to remove the mutating addSpanProcessor - there's no use for it, unless we add removeSpanProcessor to parallel logging libraries ability to configure handlers. If you have code that only calls add once, it can happen when building the SDK itself no matter what.

After adding this method, I can refactor our tests, and then deprecate the mutator. This is orthogonal to #2231

@bogdandrutu
Copy link
Member

The real motivation for this is to remove the mutating addSpanProcessor - there's no use for it, unless we add removeSpanProcessor to parallel logging libraries ability to configure handlers. If you have code that only calls add once, it can happen when building the SDK itself no matter what.

There is one problem with this approach "circular initialization dependencies". One of the reason "SpanProcessor" was not in the builder is because an implementation of a "SpanProcessor" (or an exporter that is needed to construct the SpanProcessor) may need/depend on a "[Tracer/Meter]Provider" to record metrics or some tracing.

Now the problem is:

  • If implementation relies on the "global" then they will statically initialize their "Meter/Tracer" with a no-op or via SPI, and all their telemetry will not be correctly recorded.
  • If they accept a provider during construction you don't have one because you are constructing it.

So how are you going to solve this initialization ordering issue?

@anuraaga
Copy link
Contributor Author

anuraaga commented Dec 9, 2020

If implementation relies on the "global" then they will statically initialize their "Meter/Tracer" with a no-op or via SPI, and all their telemetry will not be correctly recorded.

Hmm - it seems the problem already exists when trying to configure the global OpenTelemetry, there's an implicit order that the constructor (or maybe even just classloading) can't be called before calling the mutable addSpanProcessor.

Have also gotten feedback from teammates that it's very mysterious having to do a mutation, not being able to configure the SDK in one go.

So need to come up with something I think :)

The issue seems to mostly be about details of tracing / monitoring (exporters, remote samplers, etc). So it would be good if we can push the complexity down from the end-user layer (OpenTelemetry / OpenTelemetrySdk) to the observability layer, the exporters (well span processors) themselves as an example. With a DI framework, I'd just inject a lazy meter into and I think this will be fairly common for custom logic in apps. For us, we can have setters on the exporters and call them directly. Or we could generalize a little bit with interface like LazyTracerInitializer or such which builders are aware of. Using observability within one of the dependencies of observability is a special case so I think there are options where we can shift the complexity away from the actual entrypoint of OpenTelemetry.

@bogdandrutu
Copy link
Member

The problem is that then you make creating of other telemetry components more of a special case, if you don't care about that, then this may be a decent solution.

A second option is to always rely on SPI to load the core parts of the sdk, then the current OtelSDK.Builder is not a builder, it just sets the configuration to the already loaded core part.

From user perspective this happens in one shot, and it also allows telemetry components (except core part loaded with SPI) to access the global correctly.

That means user does not need to set the global when using our SDK which is also fine I think.

@jkwatson
Copy link
Contributor

ok, I think we have a plan to go forward with this now.

  1. Add this option to the tracer provider builder
  2. remove all non-component level options from the opentelemetry sdk builder

I'm ok with having 1 happen in a separate PR from 2. @bogdandrutu Your thoughts on sequencing?

Anuraag Agrawal added 3 commits December 15, 2020 15:01
@anuraaga
Copy link
Contributor Author

Is this PR blocked on anything? Let me know if there're any changes

Copy link
Contributor

@jkwatson jkwatson left a comment

Choose a reason for hiding this comment

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

Thanks!

@jkwatson
Copy link
Contributor

@bogdandrutu is there anything left to block this, since we removed the options from the main sdk builder itself?

@bogdandrutu bogdandrutu merged commit fb5b497 into open-telemetry:master Dec 18, 2020
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.

3 participants