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

JIT exporter metrics #4993

Merged
merged 5 commits into from
Dec 17, 2022
Merged

Conversation

jack-berg
Copy link
Member

ExporterMetrics standardizes metric collection from exporters. Instrumenting exporters presents a chicken and the egg problem since exporters are part of the SDK, yet rely on the SDK for instrumentation.

The effort to split out exporter configuration into SPI implementations in #4949 makes this more clear as exporter configuration code (like jaeger configuration) sets the meter provider in a way that SPI implementations can't because they don't have access to the meter provider.

IMO instrumenting exporters is one of the few cases where depending on GlobalOpenTelemetry makes sense: Rather than worry about the ordering of initializing SdkMeterProvider and passing it to other exporters, just have those exporters instrument themselves using GlobalOpenTelemetry#getMeterProvider(). The one caveat is that instrumentation has to be "just in time", ensuring that GlobalOpenTelemetry#set has been called before instrumentation begins.

This PR adjusts ExporterMetrics instrumentation to be "just in time". It also adjusts all the exporters to use GlobalOpenTelemetry.getMeterProvider by default instead of MeterProvider.noop().

@jack-berg jack-berg requested a review from a team November 28, 2022 20:07
@codecov
Copy link

codecov bot commented Nov 28, 2022

Codecov Report

Base: 91.24% // Head: 91.26% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (6979bbf) compared to base (7c6f1bd).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #4993      +/-   ##
============================================
+ Coverage     91.24%   91.26%   +0.02%     
- Complexity     4894     4900       +6     
============================================
  Files           552      552              
  Lines         14481    14497      +16     
  Branches       1386     1388       +2     
============================================
+ Hits          13213    13231      +18     
  Misses          878      878              
+ Partials        390      388       -2     
Impacted Files Coverage Δ
...exporter/jaeger/JaegerGrpcSpanExporterBuilder.java 80.64% <ø> (ø)
...r/otlp/http/trace/OtlpHttpSpanExporterBuilder.java 100.00% <ø> (ø)
...porter/otlp/trace/OtlpGrpcSpanExporterBuilder.java 100.00% <ø> (ø)
...lp/http/logs/OtlpHttpLogRecordExporterBuilder.java 100.00% <ø> (ø)
...er/otlp/logs/OtlpGrpcLogRecordExporterBuilder.java 100.00% <ø> (ø)
...entelemetry/exporter/internal/ExporterMetrics.java 100.00% <100.00%> (ø)
...ry/exporter/internal/grpc/GrpcExporterBuilder.java 98.78% <100.00%> (+0.01%) ⬆️
...try/exporter/internal/grpc/OkHttpGrpcExporter.java 80.85% <100.00%> (+0.20%) ⬆️
...y/exporter/internal/grpc/UpstreamGrpcExporter.java 90.24% <100.00%> (ø)
...metry/exporter/internal/okhttp/OkHttpExporter.java 95.45% <100.00%> (ø)
... and 8 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

what will happen if a user has configured OpenTelemetrySdk without registering it global?

@jack-berg
Copy link
Member Author

what will happen if a user has configured OpenTelemetrySdk without registering it global?

They have to manually call setMeterProvider on their respective exporters.

@trask
Copy link
Member

trask commented Nov 29, 2022

They have to manually call setMeterProvider on their respective exporters.

I guess, I'm wondering what happens if they don't do this, will the GlobalOpenTelemetry usage force another sdk to be auto-initialized?

@jack-berg
Copy link
Member Author

if they don't do this, will the GlobalOpenTelemetry usage force another sdk to be auto-initialized?

Ugh.. yes. Geez I'm really not a fan of GlobalOpenTelemetry#maybeAutoconfigureAndSetGlobal.

@jack-berg
Copy link
Member Author

if they don't do this, will the GlobalOpenTelemetry usage force another sdk to be auto-initialized?

We could have a separate "global" meter that autoconfigure sets. For example, ExporterMetrics could have a setGlobalMeterProvider() method which is used if a user hasn't explicitly configured a meter via one of the setMeterProvider methods on the exporter builders (i.e. OtlpGrpcSpanExporterBuilder#setMeterProvider). This would have to be used in tandem with the just in time approach.

This would work well for export providers which are built in to the core SDK, and can take advantage of special behavior in the autoconfigure module.

But the more generalized problem is how do SDK extension components (exporters, processors, etc) get access to OpenTelemetry so they can instrument themselves? GlobalOpenTelemetry has been our answer to tricky ordering and access situations, and seems to be the right tool for the job here. The fact that it has a side affect where GlobalOpenTelemetry#get wants to initialize AutoConfiguredOpenTelemetrySdk really throws a wrench into the scenario where someone wants to use AutoConfiguredOpenTelemetrySdkBuilder.setResultAsGlobal(false).

Couple of ideas come to mind:

  • Remove the AutoConfiguredOpenTelemetrySdk initialization side affect from GlobalOpenTelemetry. This seems controversial, but I'm not exactly sure why because I'm not sure who / what instrumentation relies on that side affect.
  • When AutoConfiguredOpenTelemetrySdkBuilder.setResultAsGlobal(false), set GlobalOpenTelemetry to noop if it hasn't already been set.
  • When AutoConfiguredOpenTelemetrySdkBuilder.setResultAsGlobal(false), set a flag on GlobalOpenTelemetry that prevents future calls to GlobalOpenTelemetry.get from initializing AutoConfiguredOpenTelemetrySdk.

@mateuszrzeszutek
Copy link
Member

  • Remove the AutoConfiguredOpenTelemetrySdk initialization side affect from GlobalOpenTelemetry. This seems controversial, but I'm not exactly sure why because I'm not sure who / what instrumentation relies on that side affect.

I prefer this option. We're discouraging everybody from using the GlobalOpenTelemetry in manually instrumented applications anyway, so why do we want it to be "smart" and auto-load the SDK? You don't need the global instance auto-configuring the SDK when using the agent, and when you're instrumenting the app manually you shouldn't be using the global in the first place.

@jack-berg
Copy link
Member Author

With #5010 merged we don't need to worry about side affects of calling GlobalOpenTelemetry.get(). Autoconfiguration users will only be able to get instrumentation about their exporters if they use AutoConfiguredOpenTelemetrySdk.builder().setResultAsGlobal(true) or otherwise call GlobalOpenTelemetry.set. If not, exporters will use OpenTelemetry.noop(). #5021 unlocks exporter instrumentation in scenarios where GlobalOpenTelemetry is not set, but using GlobalOpenTelemetry seems find for now given the experimental state of this telemetry.

@open-telemetry/java-approvers can you take another look?

Comment on lines +71 to +87
private LongCounter seen() {
LongCounter seen = this.seen;
if (seen == null) {
seen = meter().counterBuilder(exporterName + ".exporter.seen").build();
this.seen = seen;
}
return seen;
}

private LongCounter exported() {
LongCounter exported = this.exported;
if (exported == null) {
exported = meter().counterBuilder(exporterName + ".exporter.exported").build();
this.exported = exported;
}
return exported;
}
Copy link
Member

Choose a reason for hiding this comment

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

it's not clear to me if this initialization should be synchronized or not, but I think it's ok either way for now and will get cleaned up by #5021 since then there will be post-init method that can be used for initialization

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't need to be synchronized. It looks a bit weird, but if two threads call exported() at the same time, both will initialize a counter and set this.exported, but this isn't a problem.

@jack-berg
Copy link
Member Author

@jkwatson any thoughts on this?

@jkwatson
Copy link
Contributor

@jkwatson any thoughts on this?

Ship it

@jack-berg jack-berg merged commit 551e764 into open-telemetry:main Dec 17, 2022
dmarkwat pushed a commit to dmarkwat/opentelemetry-java that referenced this pull request Dec 30, 2022
* Build ExporterMetrics instruments just in time

* Exporters use GlobalOpenTelemetry#getMeterProvider() if meter provider is not set

* FullConfigTest reset GlobalOpenTelemetry

* MetricExporters use MeterProvider.noop()
@zeitlinger
Copy link
Member

We're discouraging everybody from using the GlobalOpenTelemetry in manually instrumented applications anyway

What is the recommended approach?

@jkwatson
Copy link
Contributor

jkwatson commented Jan 9, 2023

We're discouraging everybody from using the GlobalOpenTelemetry in manually instrumented applications anyway

What is the recommended approach?

Inject an instance of OpenTelemetry into the places where it's needed.

@zeitlinger
Copy link
Member

Inject an instance of OpenTelemetry into the places where it's needed.

Let me add some context: If I'm writing a library such as riptide - what should I do?

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.

5 participants