-
Notifications
You must be signed in to change notification settings - Fork 873
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
Library instrumentation should read its version from a file #5692
Library instrumentation should read its version from a file #5692
Conversation
try { | ||
List<URL> urls = | ||
list( | ||
new ResourceLoaderProxy() |
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.
Do I understand correctly that for library instrumentation this class needs to be in the same class loader as the library instrumentation so that it can find the versions file?
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.
Yep - which I think should normally happen: library instrumentation triggers Instrumenter
load, Instrumenter
triggers this.
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.
What if you are using a library instrumentation along with the agent. Then this class will be in boot loader and won't find the library versions.
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.
The shaded version of this class (provided by the agent) will be in the bootloader; the unshaded version (provided by the application itself) should still see its own versions file (+ all other versions files, since they get resource-injected into all classloaders).
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.
Completely forgot that this would also get shaded. I still think it is possible to break it by placing instrumentation-api in parent class loader and library instrumentation in child class loader.
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.
Yes, that scenario would break this mechanism. I wonder how common it is though, you'd have to deliberately separate the instrumentation-api from the instrumentation.
Since we expect either bundled javaagent or use of the BOM for aligned versions, can we just have one version instead of per-instrumentation? Can just be tied straight into instrumentation-api. I guess we want to separate though since it is precisely finding out about such a versionoing issue that we want the files present though. In general, any approach that involves a single file can cause issues with fat jars, so we would want a unique filename per artifact (basically putting name into the file path instead of in the file contents). Service loader doesn't quite solve the issue with fat jars but is still better supported than arbitrary files so it could be an alternative if needing to stick with a single file path. |
a401183
to
f9045d1
Compare
...in/java/io/opentelemetry/instrumentation/api/internal/EmbeddedInstrumentationProperties.java
Outdated
Show resolved
Hide resolved
private static final String INSTRUMENTATION_NAME = "io.opentelemetry.jedis-3.0"; | ||
private static final String INSTRUMENTATION_NAME = "io.opentelemetry.jedis-4.0"; |
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.
💯
private static final String INSTRUMENTATION_NAME = "io.opentelemetry.ratpack-1.4"; | ||
private static final String INSTRUMENTATION_NAME = "io.opentelemetry.ratpack-1.7"; |
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 there a test now that fails when these are wrong?
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.
No, there isn't, unfortunately -- I went over all usages of Instrumenter#builder()
and fixed them all manually. Currently I don't have any good idea about how to verify this in a smart way that does not require adding assertions to every test.
@@ -80,6 +83,7 @@ | |||
* different library versions it's easy to find out which instrumentations produced the telemetry | |||
* data. | |||
*/ | |||
// TODO: add a setInstrumentationVersion method to the builder instead |
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.
👍
conventions/src/main/kotlin/io.opentelemetry.instrumentation.base.gradle.kts
Show resolved
Hide resolved
conventions/src/main/kotlin/io.opentelemetry.instrumentation.base.gradle.kts
Outdated
Show resolved
Hide resolved
...in/java/io/opentelemetry/instrumentation/api/internal/EmbeddedInstrumentationProperties.java
Outdated
Show resolved
Hide resolved
conventions/src/main/kotlin/io.opentelemetry.instrumentation.base.gradle.kts
Show resolved
Hide resolved
...in/java/io/opentelemetry/instrumentation/api/internal/EmbeddedInstrumentationProperties.java
Outdated
Show resolved
Hide resolved
...in/java/io/opentelemetry/instrumentation/api/internal/EmbeddedInstrumentationProperties.java
Outdated
Show resolved
Hide resolved
...in/java/io/opentelemetry/instrumentation/api/internal/EmbeddedInstrumentationProperties.java
Outdated
Show resolved
Hide resolved
1306341
to
ba65f6a
Compare
❤️ |
…emetry#5692) * Library instrumentation should read its version from a file * errorprone * animalsniffer * code review comments * add name as task input too * code review comments
Resolves #2761. As discussed on the last SIG meeting, each instrumentation gets its separate version file.