Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

If there's a way to keep traces and disable metrics, this could be interesting in the context of Trino but I'm not sure it's possible and also in a auto-instrumentation context I guess people would expect to have the OkHttp metrics.

FYI I've sent the files for the CLA, not sure how long it will take to get approved. I believe this PR still makes sense.

I'm not sure this change is correct. We have a compile time dependency on opentelemetry-okhttp-3.0 which has a compile time dependency on opentelemetry-instrumentation-api which in turn has a runtime dependency on opentelemetry-extension-incubator. I assume that the instrumentation code will fail at runtime if that dependency is not present.

In NonRegisteringTrinoDriver, we try to instantiate OkHttpTelemetry while ignoring NoClassDefFoundError. Is that code guaranteed to throw NoClassDefFoundError if opentelemetry-extension-incubator is not present on the classpath? And can the shaded instrumentation code work correctly with a non-shaded version of opentelemetry-extension-incubator?

At a minimum, we would need integration tests to verify that the JDBC driver still works correctly when the OpenTelemetry API is on the classpath but the extension JAR is not.

I also don't understand the actual problem here. We're shading both io.opentelemetry.extension and io.opentelemetry.instrumentation, so the instanceof checks in HttpMetricsAdvice should work. This could be verified in a debugger.

Ideally, we'd like to have both trace propagation and HTTP client traces when the OpenTelemetry API is on the classpath.

We've never considered OpenTelemetry metrics as we don't use these in Trino, so the fact that these are collected is accidental. Someone such as yourself who is more familiar with how metrics should work will need to help to make sure they are working appropriately. For now, it's probably best to disable them, since it sounds like they are more harmful than helpful.

The nature of JDBC drivers introduces constraints that necessitates the shading and complex dependency handling:

• Must work standalone, without OpenTelemetry or any other dependency on the classpath. Applications will simply add the JAR to their classpath and expect it to work. There is no facility for requiring external dependencies.
• Must not conflict with an application's own dependencies. This is why OkHttp, etc., are shaded. Otherwise, for example, the driver would conflict with an application that used its own version of OkHttp.
• Must not depend on a specific version of OpenTelemetry. We expect/hope that the core OpenTelemetry API is stable and won't conflict with whatever version an application happens to be using. This isn't true for instrumentation, extensions, etc., which are rapidly changing, so we shade those.

If we can't easily disable metrics collection, we might be forced to remove the OkHttp instrumentation and instead do manual trace propagation (and HTTP client trace collection if that is useful).

Thanks for the feedbacks @electrum

Is that code guaranteed to throw NoClassDefFoundError if opentelemetry-extension-incubator is not present on the classpath?
(...)
At a minimum, we would need integration tests to verify that the JDBC driver still works correctly when the OpenTelemetry API is on the classpath but the extension JAR is not.

I'll check that.

And can the shaded instrumentation code work correctly with a non-shaded version of opentelemetry-extension-incubator?

I believe the manual integration tests I did are proving this works.

I also don't understand the actual problem here. We're shading both io.opentelemetry.extension and io.opentelemetry.instrumentation, so the instanceof checks in HttpMetricsAdvice should work. This could be verified in a debugger.

I can run the reproduction case and provide debugging details for this.

We've never considered OpenTelemetry metrics as we don't use these in Trino, so the fact that these are collected is accidental. Someone such as yourself who is more familiar with how metrics should work will need to help to make sure they are working appropriately. For now, it's probably best to disable them, since it sounds like they are more harmful than helpful.

I can give it more time to see if we could entirely disable metrics for the OkHttp calls made by Trino.
However I'm definitely not that familiar with this 🫤

I also don't understand the actual problem here. We're shading both io.opentelemetry.extension and io.opentelemetry.instrumentation, so the instanceof checks in HttpMetricsAdvice should work. This could be verified in a debugger.

FYI, I had a bit of time to debug the instanceof checks. I confirm it doesn't work because of shading.

The call stack is as follows:

applyOldClientDurationAdvice:40, HttpMetricsAdvice (io.trino.jdbc.$internal.opentelemetry.instrumentation.api.instrumenter.http)
<init>:69, HttpClientMetrics (io.trino.jdbc.$internal.opentelemetry.instrumentation.api.instrumenter.http)
create:-1, HttpClientMetrics$$Lambda/0x00007fa400285630 (io.trino.jdbc.$internal.opentelemetry.instrumentation.api.instrumenter.http)
buildOperationListeners:320, InstrumenterBuilder (io.trino.jdbc.$internal.opentelemetry.instrumentation.api.instrumenter)
<init>:92, Instrumenter (io.trino.jdbc.$internal.opentelemetry.instrumentation.api.instrumenter)
create:-1, InstrumenterBuilder$InstrumenterConstructor$$Lambda/0x00007fa400286690 (io.trino.jdbc.$internal.opentelemetry.instrumentation.api.instrumenter)
buildInstrumenter:284, InstrumenterBuilder (io.trino.jdbc.$internal.opentelemetry.instrumentation.api.instrumenter)
buildInstrumenter:276, InstrumenterBuilder (io.trino.jdbc.$internal.opentelemetry.instrumentation.api.instrumenter)
create:62, OkHttpInstrumenterFactory (io.trino.jdbc.$internal.opentelemetry.instrumentation.okhttp.v3_0.internal)
build:112, OkHttpTelemetryBuilder (io.trino.jdbc.$internal.opentelemetry.instrumentation.okhttp.v3_0)
instrumentClient:69, NonRegisteringTrinoDriver (io.trino.jdbc)
connect:62, NonRegisteringTrinoDriver (io.trino.jdbc)
...

Here's a screenshot of the behaviour in IntelliJ debugger when being at the HttpMetricsAdvice#applyOldClientDurationAdvice method which is the one supposed to add the advices filtering the noisy metrics:

You can see in the bottom section that:

• the builder parameter is not shaded, it's a io.opentelemetry.sdk.metrics.SdkDoubleHistogramBuilder extends io.opentelemetry.extension.incubator.metrics.ExtendedDoubleHistogramBuilder, belongs to opentelemetry-sdk-metrics
• but the instanceof is checking against the shaded io.trino.jdbc.$internal.opentelemetry.extension.incubator.metrics.ExtendedDoubleHistogramBuilder
• the same instanceof check against the non shaded ExtendedDoubleHistogramBuilder would work as intended

If you're not willing to accept this current PR (I don't have enough knowledge or expertise on the requirements for a JDBC driver, I'd need help from someone else to check further if the change doesn't bring unexpected issues), I don't see any easy way out of the situation excepted to remove OpenTelemetry instrumentation in favor of a "home made" manual trace propagation if that's the initial reason for the integration of OpenTelemetry.

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

In NonRegisteringTrinoDriver, we try to instantiate OkHttpTelemetry while ignoring NoClassDefFoundError. Is that code guaranteed to throw NoClassDefFoundError if opentelemetry-extension-incubator is not present on the classpath?

yes, it is

And can the shaded instrumentation code work correctly with a non-shaded version of opentelemetry-extension-incubator?

yes, it can

I also don't understand the actual problem here. We're shading both io.opentelemetry.extension and io.opentelemetry.instrumentation, so the instanceof checks in HttpMetricsAdvice should work.

You can view the classes in https://github.com/open-telemetry/opentelemetry-java/tree/main/extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/metrics as incubating api. Eventually this functionality will be moved to opentelemetry-api and the dependency on opentelemetry-extension-incubator will become unnecessary. The intended usage is that user will cast DoubleHistogramBuilder to ExtendedDoubleHistogramBuilder to use the incubating api. If you shade ExtendedDoubleHistogramBuilder such cast will be impossible because you only shade the instrumentation code, the sdk that is provided by the user will still use an unshaded version of ExtendedDoubleHistogramBuilder. This is the actual object that is being cast https://github.com/open-telemetry/opentelemetry-java/blob/f123d789097d714ed94f76d98e87985067a7b301/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkDoubleHistogram.java#L60. It is part of the sdk that you don't shade. Because of that you should not shade these classes similarly like you don't shade the opentelemetry-api.

If we can't easily disable metrics collection, we might be forced to remove the OkHttp instrumentation and instead do manual trace propagation (and HTTP client trace collection if that is useful).

Currently metrics collection can not be disabled for the okhttp client instrumentation library. Http client metrics are part of the opentelemetry specification https://opentelemetry.io/docs/specs/semconv/http/http-metrics/#http-client.

@electrum I'd really like this PR (or at least a fix to the initial problem) be solved. What more would be missing to move forward?

Should I provide an integration test that can be automated? If so, are there already some in the project?

cc @mattstep and @electrum .. please chime in here.

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time.

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time.

Sorry for the close again - I added the stale-ignore label now. Can @mattstep and @electrum please look at this PR

Will non-shaded JDBC driver work for you @gaeljw?

I'm going to add it in #22098

For the record, OpenTelemetry now workaround the initial issue by detecting unexpected class hierarchy (see open-telemetry/opentelemetry-java-instrumentation#10671).

Will non-shaded JDBC driver work for you @gaeljw?

I'm going to add it in #22098

A non-shaded JDBC driver would be nice indeed. It'll let "advanced" users manage the version conflicts themselves, likely using OTEL BOM.

This would be a "regular" non-fat JAR right? Only to be used with a proper dependency management tool like Maven.

If it's a fat JAR including all the dependencies with their original packages, this may solve this issue but I think it would bring even more issues/conflicts and harder or even impossible to fix by users.

@gaeljw Regular jar to be used with a build tool like maven, gradle or sbt

Closing as OpenTelemetry will not be shaded anymore with #23458

Thanks @gaeljw