JIT exporter metrics #4993

jack-berg · 2022-11-28T20:07:56Z

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().

…r is not set

codecov · 2022-11-28T21:49:03Z

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.

trask

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

jack-berg · 2022-11-29T22:24:58Z

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

They have to manually call setMeterProvider on their respective exporters.

trask · 2022-11-29T22:49:20Z

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 · 2022-11-29T22:52:21Z

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 · 2022-11-30T17:32:10Z

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 · 2022-12-01T11:35:13Z

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.

…y-java into jit-exporter-metrics

jack-berg · 2022-12-14T22:02:16Z

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?

.../all/src/main/java/io/opentelemetry/exporter/otlp/metrics/OtlpGrpcMetricExporterBuilder.java

...src/main/java/io/opentelemetry/exporter/otlp/http/metrics/OtlpHttpMetricExporterBuilder.java

trask · 2022-12-15T16:55:53Z

exporters/common/src/main/java/io/opentelemetry/exporter/internal/ExporterMetrics.java

+  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;
+  }


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

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 · 2022-12-17T15:29:50Z

@jkwatson any thoughts on this?

jkwatson · 2022-12-17T18:00:21Z

@jkwatson any thoughts on this?

Ship it

* Build ExporterMetrics instruments just in time * Exporters use GlobalOpenTelemetry#getMeterProvider() if meter provider is not set * FullConfigTest reset GlobalOpenTelemetry * MetricExporters use MeterProvider.noop()

zeitlinger · 2023-01-09T12:48:27Z

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

What is the recommended approach?

jkwatson · 2023-01-09T16:20:45Z

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 · 2023-01-10T08:05:42Z

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?

jack-berg added 2 commits November 28, 2022 13:51

Build ExporterMetrics instruments just in time

333b472

Exporters use GlobalOpenTelemetry#getMeterProvider() if meter provide…

e81e2de

…r is not set

jack-berg requested a review from a team November 28, 2022 20:07

jack-berg added 2 commits November 28, 2022 14:50

FullConfigTest reset GlobalOpenTelemetry

4db0ad1

MetricExporters use MeterProvider.noop()

c0ba39f

jack-berg mentioned this pull request Nov 29, 2022

Implement jaeger exporter provider #5002

Merged

trask reviewed Nov 29, 2022

View reviewed changes

jack-berg mentioned this pull request Nov 30, 2022

Implement otlp exporter providers #5003

Merged

jack-berg mentioned this pull request Dec 1, 2022

GlobalOpenTelemetry trigger of autoconfiguration is opt-in #5010

Merged

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

6979bbf

…y-java into jit-exporter-metrics

trask approved these changes Dec 15, 2022

View reviewed changes

mateuszrzeszutek approved these changes Dec 16, 2022

View reviewed changes

jkwatson approved these changes Dec 17, 2022

View reviewed changes

jack-berg merged commit 551e764 into open-telemetry:main Dec 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT exporter metrics #4993

JIT exporter metrics #4993

jack-berg commented Nov 28, 2022

codecov bot commented Nov 28, 2022 •

edited

Loading

trask left a comment

jack-berg commented Nov 29, 2022

trask commented Nov 29, 2022

jack-berg commented Nov 29, 2022

jack-berg commented Nov 30, 2022

mateuszrzeszutek commented Dec 1, 2022

jack-berg commented Dec 14, 2022

trask Dec 15, 2022

jack-berg Dec 15, 2022

jack-berg commented Dec 17, 2022

jkwatson commented Dec 17, 2022

zeitlinger commented Jan 9, 2023

jkwatson commented Jan 9, 2023

zeitlinger commented Jan 10, 2023

JIT exporter metrics #4993

JIT exporter metrics #4993

Conversation

jack-berg commented Nov 28, 2022

codecov bot commented Nov 28, 2022 • edited Loading

Codecov Report

trask left a comment

Choose a reason for hiding this comment

jack-berg commented Nov 29, 2022

trask commented Nov 29, 2022

jack-berg commented Nov 29, 2022

jack-berg commented Nov 30, 2022

mateuszrzeszutek commented Dec 1, 2022

jack-berg commented Dec 14, 2022

trask Dec 15, 2022

Choose a reason for hiding this comment

jack-berg Dec 15, 2022

Choose a reason for hiding this comment

jack-berg commented Dec 17, 2022

jkwatson commented Dec 17, 2022

zeitlinger commented Jan 9, 2023

jkwatson commented Jan 9, 2023

zeitlinger commented Jan 10, 2023

codecov bot commented Nov 28, 2022 •

edited

Loading