-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix(logging): add ability to filter MDC labels #6430
Conversation
...e-cloud-logging-logback/src/main/java/com/google/cloud/logging/logback/MDCEventEnhancer.java
Outdated
Show resolved
Hide resolved
...oud-logging-logback/src/test/java/com/google/cloud/logging/logback/MDCEventEnhancerTest.java
Outdated
Show resolved
Hide resolved
...oud-logging-logback/src/test/java/com/google/cloud/logging/logback/MDCEventEnhancerTest.java
Outdated
Show resolved
Hide resolved
...logging-logback/src/test/java/com/google/cloud/logging/logback/FilteredMVCEventEnhancer.java
Outdated
Show resolved
Hide resolved
...oud-logging-logback/src/test/java/com/google/cloud/logging/logback/MDCEventEnhancerTest.java
Outdated
Show resolved
Hide resolved
Codecov Report
Continue to review full report at Codecov.
|
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 thought of a possible behavior breaking change: if a user already added an additional LoggingEventEnhancer
, they would now lose the default label setting behavior.
@chingor13 That is a good point. I did document this behavior on the class, but I suppose it is technically breaking. Short of exposing a custom hook that only relates specifically to MDCs I don't know a good way around this. I am open to suggestions. |
What is our compatibility promise for artifacts labeled alpha? The version here is |
We can break the behavior, but we definitely need to document it in the release notes. |
I vote we take the breaking behavior and document it in the release notes. I think this implementation will offer more flexibility for the users as the library. Using |
...oud-logging-logback/src/test/java/com/google/cloud/logging/logback/MDCEventEnhancerTest.java
Show resolved
Hide resolved
The alpha status is wonky. Some artifacts are alpha, some are beta, and some are GA. |
Currently all MDC values are being written as labels in a LogEntry. This is not desirable for everyone. By adding MDCEventEnhancer a user now has the ability to easily filter which MDC entries get converted into labels. To maintain backwards compatability the default implementation is to write all entries as labels. If a user wants to filter these, they just need to extend MDCEventEnhancer, override the filtering, and add the new classpath to their logback.xml. Fixes googleapis#6320
@chingor13 Thoughts on this change? |
I am going to merge this. Once done I will cherry-pick these changes to goolgeapis/java-logging |
Hello! I'm just curious when it's being planned to merge fix to |
Currently all MDC values are being written as labels in a LogEntry.
This is not desirable for everyone. By adding MDCEventEnhancer a user
now has the ability to easily filter which MDC entries get converted
into labels. To maintain backwards compatability the default
implementation is to write all entries as labels. If a user wants to
filter these, they just need to extend MDCEventEnhancer, override the
filtering, and add the new classpath to their logback.xml.
Fixes #6320