Skip to content
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

Add Baggage to logback MDC controlled by flag #7892

Merged
merged 13 commits into from
Mar 9, 2023

Conversation

adamleantech
Copy link
Contributor

@adamleantech adamleantech commented Feb 23, 2023

The intention here is to allow users of the java agent to set a VM flag in order to be able to add values in the current Baggage context to MDC for logback. It seemed unwise to turn this on by default - if the application is configured to print all MDC contents (as it often the case with JSON output) then baggage would be logged out by default which may either bloat the logs or result in sensitive data being exposed unitentionally.

Addresses #1391 and #6708

Note that this is my first contribution to this repo, I've done my best to follow the existing approaches to things like testing but would appreciate any feedback.

@adamleantech adamleantech requested a review from a team as a code owner February 23, 2023 13:10
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 23, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx, this is a great addition!

Copy link
Member

@mateuszrzeszutek mateuszrzeszutek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @adamleantech !

adamleantech and others added 4 commits February 24, 2023 17:33
…a/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LogbackSingletons.java

Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>
@adamleantech
Copy link
Contributor Author

I'm not sure what is causing the build to fail here and I don't believe I have permissions to retrigger - would appreciate some help getting this clean and merged. @trask @mateuszrzeszutek @laurit

@laurit
Copy link
Contributor

laurit commented Mar 8, 2023

@adamleantech lately we migrated to gradle 8. Unfortunately the org.unbroken-dome.test-sets plugin doesn't work with gradle 8 and the tests using it were migrated to gradle test suites. See https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/log4j/log4j-context-data/log4j-context-data-2.17/javaagent/build.gradle.kts for a sample how the test suites look like. Also see

configurations {
named("testReceiveSpansDisabledImplementation") {
extendsFrom(configurations["testImplementation"])
}
}
for how you can copy all test dependencies to test suite.

@mateuszrzeszutek
Copy link
Member

(Closing and reopening to trigger checks)

@adamleantech adamleantech reopened this Mar 9, 2023
@adamleantech
Copy link
Contributor Author

adamleantech commented Mar 9, 2023

I believe the build should be clean now, not sure how to retrigger here, though @trask @laurit @mateuszrzeszutek

@laurit
Copy link
Contributor

laurit commented Mar 9, 2023

closing and reopening to trigger checks

@laurit laurit closed this Mar 9, 2023
@laurit laurit reopened this Mar 9, 2023
@laurit
Copy link
Contributor

laurit commented Mar 9, 2023

@adamleantech to retrigger checks you can either close and reopen the pull request or push an empty commit

@adamleantech
Copy link
Contributor Author

@laurit you can see above the I pushed a commit and closed then opened the PR and neither of these triggered a build :-(

@trask trask merged commit 96fd1d7 into open-telemetry:main Mar 9, 2023
@trask
Copy link
Member

trask commented Mar 9, 2023

thx @adamleantech!

@adamleantech
Copy link
Contributor Author

hooray!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants