Prevent duplicate telemetry when using both library and auto instrumentation #2661
Conversation
mateuszrzeszutek
requested review from
anuraaga,
iNikem,
jkwatson,
pavolloffay,
trask and
tylerbenson
as code owners
| // TODO: optimization: refactor matcher caching, only the matcher returned by this method should | ||
| // cache results, the intermediary ones don't need to cache anything | ||
| // right now every matcher returned from ClassLoaderMatcher caches separately | ||
| // this will also make those "Skipping ..." logs appear less |
There was a problem hiding this comment.
Will be done in next PR
mateuszrzeszutek
force-pushed
the
prevent-duplicate-telemetry
branch
from
443b7c7 to
6fb98a5
Compare
.../io/opentelemetry/javaagent/instrumentation/apachedubbo/v2_7/DubboInstrumentationModule.java
Show resolved
Hide resolved
| return classLoaderMatcher().and(not(libraryMatcher)); | ||
| } | ||
|
|
||
| private void logLibraryInstrumentationDetected() { |
There was a problem hiding this comment.
Mmm, if we don't shade library instrumentations, we will detect those classes from within our javaagent jar, no? Or am I asking nonsense?
There was a problem hiding this comment.
We won't, because they're in the agent classloader.
mateuszrzeszutek
force-pushed
the
prevent-duplicate-telemetry
branch
from
6fb98a5 to
4083520
Compare
|
(Rebasing to make those context-leak-failing tests pass) |
|
There's also the matter of testing this: I thought of adding a testSet that contains the library instrumentation and basically running the same test as the library instrumentation module - any duplicate telemetry would fail the tests. |
javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/InstrumentationModule.java
Show resolved
Hide resolved
javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/InstrumentationModule.java
Show resolved
Hide resolved
instrumentation/couchbase/couchbase-3.1/javaagent/couchbase-3.1-javaagent.gradle
Show resolved
Hide resolved
|
Sorry for very late review - are we confident classpath is a good enough signal to back off? For example, I might depend on a library which uses gRPC, and actually initializes gRPC library instrumentation itself as part of its own best practices, but that doesn't mean my own handlers will have it applied. So I thought we were going to need to go with an approach where the agent detects whether the library instrumentation is actually being used, not just the class presence. |
|
ah, that's a good point |
|
Oh, that is an excellent point... And the worst thing is that there's no reasonable workaround for that scenario: you can't exclude the grpc library instrumentation because the library that uses it will break. Hmm, I'll probably revert this and go back to the drawing board. |
Fixes #903
I've removed the library instrumentation shading (
io.opentelemetry.instrumentation.*); instrumentation-api is still shaded though (io.opentelemetry.instrumentation.api.*).Since now the agent will disable the javaagent instrumentation when it encounters a library class I think that this offers a similar level of safety as shading everything.