OpenTracing Shim: Update Tracer.close()

[carlosalberto]

Fixes #5081

As per the 1.17 Specification, the OpenTracing Tracer Close operation was added.

I feel tempted to deprecate the OpenTracingShim.createTracerShim(OpenTelemetry) method overload, as it wraps the actually TracerProvider with ObfuscatedTracerProvider, which prevents checking whether the actual instance implements Closeable or not.

[codecov]

Codecov Report

Patch coverage: 64.00% and project coverage change: -0.23 ⚠️

Comparison is base (6edba79) 91.14% compared to head (3b11d87) 90.91%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5151      +/-   ##
============================================
- Coverage     91.14%   90.91%   -0.23%     
+ Complexity     4911     4884      -27     
============================================
  Files           547      550       +3     
  Lines         14544    14476      -68     
  Branches       1399     1370      -29     
============================================
- Hits          13256    13161      -95     
- Misses          891      917      +26     
- Partials        397      398       +1     

Impacted Files	Coverage Δ
...a/io/opentelemetry/opentracingshim/TracerShim.java	69.23% <62.50%> (-8.90%)	⬇️
...opentelemetry/opentracingshim/OpenTracingShim.java	100.00% <100.00%> (ø)
...telemetry/opentracingshim/NoopSpanBuilderShim.java	33.33% <0.00%> (-66.67%)	⬇️
...ry/sdk/metrics/internal/aggregator/Aggregator.java	33.33% <0.00%> (-54.17%)	⬇️
...trics/internal/exemplar/NoopExemplarReservoir.java	60.00% <0.00%> (-40.00%)	⬇️
...metrics/internal/view/NoopAttributesProcessor.java	75.00% <0.00%> (-25.00%)	⬇️
...y/sdk/metrics/internal/exemplar/ReservoirCell.java	90.47% <0.00%> (-9.53%)	⬇️
...xporter/internal/okhttp/OkHttpExporterBuilder.java	83.63% <0.00%> (-9.47%)	⬇️
.../metrics/internal/aggregator/AggregatorHandle.java	88.23% <0.00%> (-4.87%)	⬇️
...dk/metrics/internal/aggregator/DropAggregator.java	35.71% <0.00%> (-4.29%)	⬇️
... and 83 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.

[jack-berg]

@carlosalberto the spec seems to contradict itself:

First it says:

This functionality will be provided as a bridge layer implementing the using the OpenTelemetry API. This layer MUST NOT rely on implementation specific details of any SDK.

Then later it says:

If this operation is implemented for a specific OpenTracing language, it MUST close the underlying TracerProvider if it implements a "closeable" interface or method; otherwise it MUST be defined as a no-op operation.

But the API doesn't define a close / shutdown method on TracerProvider. Its only the SDK TracerProvider that implements this. Could resolve this by having the opentracing shim accept OpenTelemetrySdk instead of OpenTelemetry, but not sure how important it is that the shim doesn't depend on the SDK.

I feel tempted to deprecate the OpenTracingShim.createTracerShim(OpenTelemetry) method overload, as it wraps the actually TracerProvider with ObfuscatedTracerProvider

Not sure this would solve the problem because the usage pattern would just shift to OpenTracingShim.createTracerShim(openTelemetry.getTracerProvider(), which would return an obfuscated tracer provider anyway.

[carlosalberto]

Its only the SDK TracerProvider that implements this. Could resolve this by having the opentracing shim accept OpenTelemetrySdk instead of OpenTelemetry, but not sure how important it is that the shim doesn't depend on the SDK.

So basically we don't want to tie ourselves to using a specific SDK - hence more the duck type alike approach of trying to go with the Closeable check (which is only a guess - also, if there is ever an alternative implementation, and such implementation's TracerProvider supports Closeable, fantastic; else, nothing we can do).

Not sure this would solve the problem because the usage pattern would just shift to OpenTracingShim.createTracerShim(openTelemetry.getTracerProvider(), which would return an obfuscated tracer provider anyway.

Good call, yes. I think we should recommend, for Java specifically, that the users pass the fresh, newly created Tracer provider instance instead.

Let me know what you think and I will update the PR accordingly.

(I will probably push that TracerProvider ends up implementing Closeable, which may be useful anyway)

cc @yurishkuro

[carlosalberto]

@jkwatson maybe you have some opinion on the matter? No problem if you don't 😄

[jkwatson]

No strong opinion, aside from the fact that I don't think we want the shim to depend on the SDK.

I wouldn't object strongly to the TracerProvider interface extending Closeable if that makes things easier for interop. We'd need to define clearly what it would mean for an alternative implementation to implement the close() method.

[jack-berg]

I don't like the idea of adding close() to TracerProvider / MeterProvider / LoggerProvider because closing is not something we want instrumentation authors to do. Not sure of the history of why OpenTracing's Tracer can be closed, but I don't think we should perpetuate that.

I'm in favor of the duck typing approach where we check if the TracerProvider is closeable. I think limiting the API to "fresh, newly created Tracer provider instance instead" is going to confuse and cause issues eventually. Would prefer if we made special accommodations to check if the instance is "ObfuscatedTracerProvider", and if so, use reflection or an internal API to get access to the raw SdkTracerProvider and close it.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[jack-berg]

I've pushed a commit to unobfuscate first if its SdkTracerProvider. This allows close() to work when OpenTracingShim.createTracerShim(GlobalOpenTelemetry.get()) is used.

I'm happy to merge this if folks are happy.

[yurishkuro]

I'm confused by this method.

• in what sense is a provider "obfuscated"?
• if the class name equals to ObfuscatedTracerProvider, why not just cast to it and ask for the delegate?

[jack-berg]

Check out the source for OpenTelemetrySdk.

OpenTelemetrySdk implements OpenTelemetry. The idea is that we don't want folks doing stuff like this:

OpenTelemetry openTelemetry = ...
SdkTracerProvider sdkTracerProvider = (SdkTracerProvider) openTelemetry.getTracerProvider();
sdkTracerProvider.shutdown();

If working in instrumentation code and all you have access to is OpenTelemetry (i.e. not OpenTelemetrySdk), you shouldn't be able to cast and call SDK specific methods.

The obfuscation layer adds friction to discourage this pattern. Though, as we see in this PR, its still possible via reflection.

[jack-berg]

@jkwatson / @carlosalberto any reason not to merge this for the upcoming release?