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

fix(logging): add ability to filter MDC labels #6430

Merged
merged 5 commits into from
Nov 14, 2019

Conversation

codyoss
Copy link
Member

@codyoss codyoss commented Oct 3, 2019

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

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 3, 2019
@codyoss codyoss requested a review from a team October 3, 2019 15:31
@codecov
Copy link

codecov bot commented Oct 3, 2019

Codecov Report

Merging #6430 into master will increase coverage by 0.26%.
The diff coverage is 90.9%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #6430      +/-   ##
============================================
+ Coverage     46.35%   46.61%   +0.26%     
- Complexity    27995    28428     +433     
============================================
  Files          2613     2615       +2     
  Lines        288066   290015    +1949     
  Branches      33776    34179     +403     
============================================
+ Hits         133524   135200    +1676     
- Misses       144303   144502     +199     
- Partials      10239    10313      +74
Impacted Files Coverage Δ Complexity Δ
.../google/cloud/logging/logback/LoggingAppender.java 68.5% <100%> (+4.72%) 30 <3> (+2) ⬆️
...google/cloud/logging/logback/MDCEventEnhancer.java 83.33% <83.33%> (ø) 4 <4> (?)
.../com/google/cloud/bigquery/spi/v2/BigQueryRpc.java 71.42% <0%> (-9.53%) 0% <0%> (ø)
.../main/java/com/google/cloud/bigquery/BigQuery.java 72% <0%> (-5.56%) 0% <0%> (ø)
...google/cloud/bigquery/ExternalTableDefinition.java 56% <0%> (-2.7%) 18% <0%> (+2%)
...c/main/java/com/google/cloud/bigquery/Routine.java 80.59% <0%> (-1.76%) 18% <0%> (+7%)
...google/cloud/bigquery/StandardTableDefinition.java 90.69% <0%> (-1.49%) 15% <0%> (+1%)
.../java/com/google/cloud/bigquery/JobStatistics.java 92.54% <0%> (-1.38%) 29% <0%> (+16%)
...c/main/java/com/google/cloud/bigquery/Dataset.java 87.8% <0%> (-0.91%) 27% <0%> (+11%)
...main/java/com/google/cloud/firestore/Internal.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43ca154...dd26744. Read the comment docs.

Copy link
Contributor

@chingor13 chingor13 left a 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.

@codyoss
Copy link
Member Author

codyoss commented Oct 3, 2019

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.

@codyoss
Copy link
Member Author

codyoss commented Oct 3, 2019

What is our compatibility promise for artifacts labeled alpha? The version here is 0.111.1-alpha-SNAPSHOT.

@chingor13
Copy link
Contributor

We can break the behavior, but we definitely need to document it in the release notes.

@codyoss
Copy link
Member Author

codyoss commented Oct 3, 2019

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 LoggingEventEnhancer is not well documented in our examples today so I anticipate little breakage. The commit that introduced this MDC feature fbb4776 was not from too far back.

@codyoss codyoss requested a review from chingor13 October 3, 2019 20:45
@elharo
Copy link
Contributor

elharo commented Oct 5, 2019

The alpha status is wonky. Some artifacts are alpha, some are beta, and some are GA.

@codyoss codyoss added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 8, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 8, 2019
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
@codyoss codyoss added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 14, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 14, 2019
@codyoss
Copy link
Member Author

codyoss commented Nov 14, 2019

@chingor13 Thoughts on this change?

@codyoss
Copy link
Member Author

codyoss commented Nov 14, 2019

I am going to merge this. Once done I will cherry-pick these changes to goolgeapis/java-logging

@codyoss codyoss merged commit 6c40d9b into googleapis:master Nov 14, 2019
@codyoss codyoss deleted the 6320-mdc branch November 14, 2019 16:40
@gendolf3d
Copy link

Hello! I'm just curious when it's being planned to merge fix to goolgeapis/java-logging project?
Thank you in advance!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add possibility to control MDC values for adding to log message
7 participants