-
Notifications
You must be signed in to change notification settings - Fork 867
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Load couchbase otel instrumentation for 3.1 #2524
Load couchbase otel instrumentation for 3.1 #2524
Conversation
|
||
public class CouchbaseEnvironmentInstrumentation implements TypeInstrumentation { | ||
@Override | ||
public ElementMatcher<ClassLoader> classLoaderOptimization() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really optimizing anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops - at first was going to do a subclass check but realized I shouldn't need it
@Override | ||
protected String[] additionalHelperClassNames() { | ||
return new String[] { | ||
"com.couchbase.client.tracing.opentelemetry.OpenTelemetryRequestSpan", | ||
"com.couchbase.client.tracing.opentelemetry.OpenTelemetryRequestTracer" | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is interesting: if we're going to include external library instrumentations in the javaagent (and we're probably going to do that, since it seems to be a good way to make the agent a bit slimmer & simpler) then we need to improve muzzle a bit.
Right now muzzle not only does not collect helper classes from 3rd party packages, it also collects no references whatsoever. Which means that we actually don't have the muzzle check both in build time and runtime.
It should be possible to define the set of packages for each module, by default it'd be same as InstrumentationClassPredicate
- I'm going to open an issue for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out there already was one: #1395
I've just added my 3 cents there
As we added to #1395 will go ahead and merge this |
tracing-opentelemetry-0.3.3
only supports couchbase 3.1, not 3.0. It seems quite tricky to deal with external library instrumentation if they have version alignment requirements, which is fairly reasonable. Let's see how it goes I guess.Fixes #2520