Disable Elasticsearch instrumentation for ES clients 8.10+

[AlexanderWert]

The Elasticsearch client introduces native, on-by-default OTel instrumentation in version 8.10.

Therefore, the agent-based instrumentation should be disabled for those versions of the Elasticsearch client.

[mateuszrzeszutek]

Nice! 👍

[trask]

What do you think of adding a latestDepTestLibrary dependency in order to limit the version we test against to 8.9.+?

(it's surprising the our tests appear to pass against 8.10+ with the native instrumentation, but guessing they may stop passing at some point as you enhance the native instrumentation)

[AlexanderWert]

What do you think of adding a latestDepTestLibrary dependency in order to limit the version we test against to 8.9.+?

(it's surprising the our tests appear to pass against 8.10+ with the native instrumentation, but guessing they may stop passing at some point as you enhance the native instrumentation)

Yes, need to adapt tests and add a test for 8.10+ versions ensuring those provide telemetry and the instrumentation being disabled. 8.10 is not released yet, I guess that's why the tests still pass. We can wait with merging this PR until 8.10 is released and I can add the tests then.

[trask]

8.10 is not released yet, I guess that's why the tests still pass.

ah, I missed that, thx

We can wait with merging this PR until 8.10 is released

👍

I can add the tests then

let's decide first whether we want to add tests in this repo for native instrumentation, I'll add this topic to the Java SIG agenda this week

[trask]

let's decide first whether we want to add tests in this repo for native instrumentation, I'll add this topic to the Java SIG agenda this week

@AlexanderWert we discussed this, but in the end I think want to leave it up to you in this particular case, check out the discussion starting at 47:15 in this recording: https://zoom.us/rec/share/L_HeZkVwtNsb_7Tjhr5-yBKACDtxcMJ-K4ug3MXq0YrW-F_XtP63iqbrhtePgAAt.bwuP1Me0aulyGMGV

[AlexanderWert]

let's decide first whether we want to add tests in this repo for native instrumentation, I'll add this topic to the Java SIG agenda this week

@AlexanderWert we discussed this, but in the end I think want to leave it up to you in this particular case, check out the discussion starting at 47:15 in this recording: https://zoom.us/rec/share/L_HeZkVwtNsb_7Tjhr5-yBKACDtxcMJ-K4ug3MXq0YrW-F_XtP63iqbrhtePgAAt.bwuP1Me0aulyGMGV

@trask @mateuszrzeszutek @laurit (and others involved) Thank you for the great discussion in the Java SIG.
The discussion helped a lot and triggered some thoughts (some of them not directly related):

1. I think for now we will keep the testing in the ES clients. BUT, I think we need a general discussion to come up with a strategy on how to deal with native instrumentations in a consistent way regarding multiple aspects mentioned in the SIG discussion:
   • how to encourage library vendors to do native instrumentations
   • how to ensure (or at least encourage) library vendors follow semantic conventions
   • how to ensure (or at least encourage) library vendors implement recommended features, config options, etc. (e.g. span suppression)

--> I think starting with a How-To / recommendations guide or blog post for library vendors on native instrumentation would be a great starting point. Happy to get this started, if you agree.

2. I wasn't fully aware of the span suppression feature. Looking at the code, I think I get the idea and I think we should add that to Elastic's native ES client instrumentation as an enhancement in the future. But this case let me to the question of documentation: We have great documentation on the core usage and features, but, what about advanced features like the span suppression? Is there a documentation for that somewhere else (other than the code itself)? If not, I think improving documentation on these kind of features would be of a great value.

[mateuszrzeszutek]

2. e have great documentation on the core usage and features, but, what about advanced features like the span suppression? Is there a documentation for that somewhere else (other than the code itself)? If not, I think improving documentation on these kind of features would be of a great value.

We briefly mentioned it here, but that's probably all. I 100% agree that the current documentation is hardly sufficient.

[trask]

I think starting with a How-To / recommendations guide or blog post for library vendors on native instrumentation would be a great starting point. Happy to get this started, if you agree.

I think an OpenTelemetry blog post would be great highlighting the new elasticsearch native instrumentation would be great! I think many projects are waiting to see others' experiences before wading into these waters themselves.

[AlexanderWert]

@laurit I think with the last commit (1c5fb6e) we will end up with duplicate instrumentation when using the native ES client instrumentation.

You're right that the REST client does not have a native instrumentation. BUT, the ES API client has a native instrumentation on the transport level and uses the Rest client underneath. Not disabling the REST client instrumentation will produce duplicate spans. I'm proposing to revert that last commit.

[laurit]

@AlexanderWert it was reverted because build did not pass with it https://github.com/open-telemetry/opentelemetry-java-instrumentation/actions/runs/6163034122/job/16725786333

[AlexanderWert]

@AlexanderWert it was reverted because build did not pass with it https://github.com/open-telemetry/opentelemetry-java-instrumentation/actions/runs/6163034122/job/16725786333

@laurit yeah, this is a tricky case, since the instrumentation affects two independent libraries. What do you think about keeping the muzzle definition with

  pass {
    group.set("org.elasticsearch.client")
    module.set("elasticsearch-rest-client")
    versions.set("[7.0,)")
    assertInverse.set(true)
  }

BUT, keep the classLoaderMatcher change as proposed:

  public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
    // Class `org.elasticsearch.client.RestClient$InternalRequest` introduced in 7.0.0.
    // Since Elasticsearch client version 8.10, the ES client comes with a native OTel
    // instrumentation
    // that introduced the class `co.elastic.clients.transport.instrumentation.Instrumentation`.
    // Disabling agent instrumentation for those cases.
    return hasClassesNamed("org.elasticsearch.client.RestClient$InternalRequest")
        .and(not(hasClassesNamed("co.elastic.clients.transport.instrumentation.Instrumentation")));
  }

with that, the build should pass since the muzzle check is being executed in isolation for the REST client, but when the native instrumentation is in place the rest client instrumentation would be disabled.