-
Notifications
You must be signed in to change notification settings - Fork 1k
Spring starter declarative config #14062
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
Spring starter declarative config #14062
Conversation
|
🔧 The result from spotlessApply was committed to the PR branch. |
17d949a to
f81444f
Compare
|
@laurit can you help me figure out what the problem is? |
tried to fix that with cbb070b |
|
I can't figure out what change could have caused that. Is the solution to not used ConfigProperties in the agent config? |
Let's see if it's 7116de1 |
I don't think this is going to help. |
Why is it not failing on main? |
Because on main you don't have it in boot loader. Actually I was wrong about
|
That worked - thanks a lot! |
bf6b7f8 to
9427fe6
Compare
|
🔧 The result from spotlessApply was committed to the PR branch. |
482c0be to
5fcca3f
Compare
...io/opentelemetry/instrumentation/spring/autoconfigure/SpringDeclarativeConfigProperties.java
Outdated
Show resolved
Hide resolved
...lemetry/instrumentation/spring/autoconfigure/internal/resources/DistroComponentProvider.java
Outdated
Show resolved
Hide resolved
...io/opentelemetry/instrumentation/spring/autoconfigure/SpringDeclarativeConfigProperties.java
Show resolved
Hide resolved
...io/opentelemetry/instrumentation/spring/autoconfigure/SpringDeclarativeConfigProperties.java
Show resolved
Hide resolved
.../src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/EmbeddedConfigFile.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/EmbeddedConfigFile.java
Show resolved
Hide resolved
.../src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/EmbeddedConfigFile.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/EmbeddedConfigFile.java
Show resolved
Hide resolved
.../src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/EmbeddedConfigFile.java
Outdated
Show resolved
Hide resolved
|
@laurit thanks for the review! please check again 😄 |
.../src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/EmbeddedConfigFile.java
Outdated
Show resolved
Hide resolved
...io/opentelemetry/instrumentation/spring/autoconfigure/SpringDeclarativeConfigProperties.java
Show resolved
Hide resolved
| +++ NEW CLASS: PUBLIC(+) io.opentelemetry.sdk.extension.incubator.fileconfig.DeclarativeConfigurationAccess (not serializable) | ||
| +++ CLASS FILE FORMAT VERSION: 52.0 <- n.a. | ||
| +++ NEW SUPERCLASS: java.lang.Object | ||
| +++ NEW METHOD: PUBLIC(+) STATIC(+) com.fasterxml.jackson.databind.ObjectMapper getObjectMapper() |
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 not ideal. I think we should just copy how ObjectMapper is constructed for now and revisit this after the release. Perhaps there is a way to write a test that would ensure that we have configured the mapper the same way as in the sdk or perhaps we can convince the sdk maintainers to move it into an internal class.
Also this trick will probably break for people who use jdk modules because of the split package. Idk whether that is relevant.
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.
Does it make sense to revert to the reflective access that I had before? This is not performance critical after all?
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.
I'll copy the ObjectMapper config for now - so that we can discuss that separately as you suggested.
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.
perhaps we can convince the sdk maintainers to move it into an internal class
Created open-telemetry/opentelemetry-java#7843
Perhaps there is a way to write a test that would ensure that we have configured the mapper the same way as in the sdk
This is already tested indirectly in the smoke test, which failed without those settings.
Fixes #14048