-
Notifications
You must be signed in to change notification settings - Fork 14
Enable Test_TelemetryEnhancedConfigReporting for java #5324
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
Conversation
|
|
mhlidd
left a comment
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.
Thanks for the changes! 🙏 Just a few small questions.
tests/test_telemetry.py
Outdated
| { | ||
| "origin": "jvm_prop", | ||
| "value": "true", | ||
| }, # File-based properties have a lower precedence than env_var: https://github.com/DataDog/dd-trace-java/blob/5c66a150ff3b16ebf9626c0f0170fc9715461a6b/utils/config-utils/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java#L507-L514 |
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.
| }, # File-based properties have a lower precedence than env_var: https://github.com/DataDog/dd-trace-java/blob/5c66a150ff3b16ebf9626c0f0170fc9715461a6b/utils/config-utils/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java#L507-L514 | |
| }, # File-based properties have a lower precedence than env_var (different that System Properties which have the same origin but a higher precedence than env_var): https://github.com/DataDog/dd-trace-java/blob/5c66a150ff3b16ebf9626c0f0170fc9715461a6b/utils/config-utils/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java#L507-L514 |
Not necessarily this wording, but I do feel like it makes sense to state that there is a nuance between origin and the actual source that is used.
|
|
||
| ENV JVM_ARGS='-javaagent:/app/dd-java-agent.jar' | ||
| COPY ./utils/build/docker/java/ConfigChaining.properties app/ConfigChaining.properties | ||
| ENV JVM_ARGS='-javaagent:/app/dd-java-agent.jar -Ddd.trace.config=/app/ConfigChaining.properties' |
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.
Not sure if you tested this already, but is it possible to change this in the definition of the Scenario? AKA would updating DD_TRACE_CONFIG to/app/ConfigChaining.properties work for all weblog apps? 🤔
mhlidd
left a comment
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.
One small change. Otherwise LGTM! 🚀
| # FIXME: Fails on APPSEC_BLOCKING, see APPSEC-51405 | ||
| # ENV DD_TRACE_INTERNAL_EXIT_ON_FAILURE=true | ||
|
|
||
| COPY ./utils/build/docker/java/ConfigChaining.properties app/ConfigChaining.properties |
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.
| COPY ./utils/build/docker/java/ConfigChaining.properties app/ConfigChaining.properties |
Duplicate from above
ac1c288 to
25743d5
Compare
Motivation
Support for Enhanced Config Reporting will be added to dd-trace-java v1.53.0-SNAPSHOT thanks to: DataDog/dd-trace-java#9404
Changes
Workflow
🚀 Once your PR is reviewed and the CI green, you can merge it!
🛟 #apm-shared-testing 🛟
Reviewer checklist
[<language>], double-check that only<language>is impacted by the changebuild-XXX-imagelabel is present