pass meter provider to avoid using the global otel instance #7475
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7475 +/- ##
============================================
+ Coverage 89.82% 90.01% +0.19%
- Complexity 6999 7082 +83
============================================
Files 798 803 +5
Lines 21199 21414 +215
Branches 2055 2088 +33
============================================
+ Hits 19042 19276 +234
+ Misses 1496 1476 -20
- Partials 661 662 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@jack-berg please have a look 😄 |
| @@ -47,6 +48,11 @@ public SpanProcessor create(SpanProcessorModel model, DeclarativeConfigContext c | |||
| if (batchModel.getScheduleDelay() != null) { | |||
| builder.setScheduleDelay(Duration.ofMillis(batchModel.getScheduleDelay())); | |||
| } | |||
| MeterProvider meterProvider = context.getMeterProvider(); | |||
| if (meterProvider != null) { | |||
| builder.setMeterProvider(meterProvider); | |||
There was a problem hiding this comment.
This partially solves the problem, but we still won't get the internal exporter telemetry.
The way we solve this in autoconfigure today is with the AutoConfigureListener. SDK components which are loaded via SPI and which report internal telemetry implement AutoConfigureListener and get a callback to update MeterProvider after OpenTelemetrySdk has been initialized. For example, see OtlpSpanExporterProvider.
I think what you've done here does work for SDK components which report internal telemetry and which are NOT loaded via SPI.
There was a problem hiding this comment.
That SPI would implement create and can just take the meter provider from DeclarativeConfigContext IIUC.
There was a problem hiding this comment.
Is the SPI guaranteed to be run on the same thread that set the value onto the context? If not, then we will need a memory barrier on the setting of the value into the context, I think.
| } | ||
|
|
||
| public void setMeterProvider(MeterProvider meterProvider) { | ||
| this.meterProvider = meterProvider; |
There was a problem hiding this comment.
any chance this needs to be cross-thread safe? In which case, putting a memory barrier around the assignment would be necessary.
There was a problem hiding this comment.
no, this doesn't need to be thread safe - it belongs to the set up logic that is done in a single thread at the start
.../main/java/io/opentelemetry/sdk/extension/incubator/fileconfig/DeclarativeConfigContext.java
Outdated
Show resolved
Hide resolved
| .create(model.getMeterProvider(), context) | ||
| .setResource(resource) | ||
| .build(); | ||
| context.setMeterProvider(meterProvider); |
There was a problem hiding this comment.
Maybe it would be less error prone if context.setMeterProvider call addCloseable internally.
Then builder.setMeterProvider(context.addCloseable(meterProvider)); could be just: builder.setMeterProvider(meterProvider); and we would be guaranteed that meterProvider will always be closed.
There was a problem hiding this comment.
it would work if we change the parameter type to SdkMeterProvider - but it would be harder to understand why the closeable logic is hidden here, but not for tracer and logger
| @@ -77,15 +88,6 @@ public OpenTelemetrySdk create( | |||
| .build())); | |||
| } | |||
|
|
|||
| if (model.getMeterProvider() != null) { | |||
There was a problem hiding this comment.
Was there any reason to move this code block upper? If not then it leaving it in the same place would make changes analysis a bit simpler.
There was a problem hiding this comment.
yes, the meter provider is taken out of the context for logger and tracer
|
@robsunday @jkwatson please check again |
1 similar comment
|
@robsunday @jkwatson please check again |
Fixes #7435