-
Notifications
You must be signed in to change notification settings - Fork 19
Add logging of the otel SDK configuration #735
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
base: main
Are you sure you want to change the base?
Conversation
int intDelay; | ||
if (propDelay != null && propDelay.length() > 0) { | ||
try { | ||
intDelay = Integer.parseInt(propDelay); |
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 delay mechanism is a workaround to wait for the opentelemetry SDK to be installed, right?
Have you tried triggering the logging from an AgentExtension instead of from the SDK-autoconfig mechanism?
Based on where it's called to me it looks like the SDK should be initialized properly at that time.
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.
can't an extension do all sorts including add processors, exporters, etc? If so the sdk instance isn't really "cooked" at that stage - other extensions could still be loaded - so printing it out then will give an incomplete config
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.
It's a different thing: The AgentExtension
s are executed after the SDK has been autoconfigured (based on the code I linked), and therefore after all SDK AutoConfigurationCustomizerProvider
which I assume you are referring to have been executed already.
So AgentExtension
can't modify the SDK anymore, they can just customize the ByteBuddy AgentBuilder
.
name.matches("elastic-otel-javaagent-\\d\\.\\d\\.\\d-SNAPSHOT\\.jar")); | ||
if (jar == null || jar.length != 1) { | ||
throw new IllegalStateException( | ||
"expecting exactly one agent jar file in ../agent/build/libs"); |
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 technically a cyclic dependency. The test lives in the custom
project, the custom
project is used as dependency when building the agent jar.
This test in turn adds an implicit test dependency from custom
on the agent jar. I haven't tested, but I think this would fail if you run ./gradlew clean check
?
Imo it would be cleaner to:
- Move this test to a project in
testing/integration-tests
- Have a look at this convention on how to declare a dependency on the agent project without adding it to the classpath. You can then pass the path to the agent-jar as a JVM-property to the test, so that we don't have this hardcoded path in the test.
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.
If needed, here is another example in the upstream jmx-metrics tests where the path to the agent jar is provided by gradle through an environment variable: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/jmx-metrics/library/build.gradle.kts
closes #730