-
Notifications
You must be signed in to change notification settings - Fork 970
Span kind for method instrumentation / Declarative configuration tooling #14014
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
Span kind for method instrumentation / Declarative configuration tooling #14014
Conversation
@laurit I hope this makes sense 😄 |
my preference would be for the method instrumentation to support declarative config, and only support this (and other) new features via declarative config instead of trying to encode more things into but I realize that is probably quite a bit more effort |
Yeah - that's a good point. If declarative config was ready to go, then I'd be happy to add it there. |
do you know what's missing that we still need? it feels like we're super close, maybe we could use this to motivate getting the final pieces in place? |
This is the error I get locally: The deadlock should have been avoided using https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/TransformSafeLogger.java
|
@laurit can you help with fixing the test? I get this deadlock (see above) locally - and in GH actions there's no output:
|
Root cause: in memory exporters are not injected with config file. |
@jack-berg can you check if this integration makes sense? |
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.
Seems like the right approach to me!
instrumentation/methods/javaagent/src/declarativeConfigTest/resources/declarative-config.yaml
Outdated
Show resolved
Hide resolved
instrumentation/methods/javaagent/src/declarativeConfigTest/resources/declarative-config.yaml
Show resolved
Hide resolved
...avaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/methods/MethodsConfig.java
Show resolved
Hide resolved
24bdfa3
to
63bfcbf
Compare
63bfcbf
to
02fb7b6
Compare
From the stack trace I'd guess that there is an exception thrown from advice code that is applied to |
...eclarativeConfigTest/java/io/opentelemetry/javaagent/instrumentation/methods/MethodTest.java
Show resolved
Hide resolved
.../src/main/java/io/opentelemetry/javaagent/instrumentation/methods/MethodInstrumentation.java
Outdated
Show resolved
Hide resolved
...avaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/methods/MethodsConfig.java
Outdated
Show resolved
Hide resolved
...ain/java/io/opentelemetry/javaagent/instrumentation/methods/MethodInstrumentationModule.java
Outdated
Show resolved
Hide resolved
@trask I think we can remove this from 2.17 milestone. As it is the declarative configuration isn't fully functional with the agent anyway. |
well - it should be fully functional with this PR - but there's no need to rush |
@laurit can you check again? |
a0f980d
to
7a819f7
Compare
5a7b353
to
0206024
Compare
@laurit should work now |
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.
❤️
* null} if not available | ||
*/ | ||
@Nullable | ||
DeclarativeConfigProperties getDeclarativeConfig(String instrumentationName); |
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.
may want to rename this param name in the future since we've discussed it also being "vendor" node, or "common"
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.
created #14160
Brings parity to
@WithSpan
where you can configure the span kind.Syntax example:
https://github.com/zeitlinger/opentelemetry-java-instrumentation/blob/3db95b4e48b527fa6f0c09be8c4cba8614951d99/instrumentation/methods/javaagent/build.gradle.kts#L18