feat: add custom logging provider to support structured logging for SLF4J 1.x + Logback 1.2.x#3693
feat: add custom logging provider to support structured logging for SLF4J 1.x + Logback 1.2.x#3693JoeWang1127 wants to merge 66 commits intomainfrom
Conversation
product-auto-label
Bot
added
size: l
JoeWang1127
changed the title
| for (Map.Entry<String, String> entry : mdcProperties.entrySet()) { | ||
| String fieldName = entry.getKey(); | ||
| String entryValueString = entry.getValue(); | ||
| if (fieldName == null || entryValueString == null) { |
There was a problem hiding this comment.
logstash-logback has some customization capabilities that I suspect are fairly easy to restore here, like configure specific entries in the MDC to be included or excluded and renaming keys, ref: readme:mdc-fields. It might be as easy as adding these conditions here, or a bit more involved. Can you take a look? I think it nice to have it preserved.
https://github.com/logfellow/logstash-logback-encoder/blob/76f5295f35bc44518af91bfccb07a8ecb142229c/src/main/java/net/logstash/logback/composite/loggingevent/MdcJsonProvider.java#L102-L103
There was a problem hiding this comment.
Added these features.
zhumin8
left a comment
zhumin8
left a comment
There was a problem hiding this comment.
LGTM.
Do you mind add a comment to pom about logstash.encoder.version used (7.3) so its easier to track in the future? Also add it to renovate config, I believe we do not want to this version upgraded?
| String entryValueString = entry.getValue(); | ||
| // an entry will be skipped if one of the scenario happens: | ||
| // 1. key or value is null | ||
| // 2. includeMdcKeyNames is not empty and the key is not in the list |
There was a problem hiding this comment.
super nit: for readability, do you mind making this comment the same logic as code?
JoeWang1127
changed the title
| - name: Install Maven modules | ||
| run: | | ||
| mvn install -B -ntp -DskipTests -Dclirr.skip -Dcheckstyle.skip | ||
| - name: Install logging module # this module is not part of root project. |
There was a problem hiding this comment.
Why do we want to install logging module in the main CI?
There was a problem hiding this comment.
Showcase needs to add the logging module as a dependency.
https://github.com/googleapis/sdk-platform-java/pull/3693/files#diff-3c3eb08864bc257af6721c1c900ec0cbb8568e2045b8aadc3330756a97b6ef1eR298
| package com.google.cloud.sdk.logging; | ||
|
|
||
| import ch.qos.logback.classic.spi.ILoggingEvent; | ||
| import com.fasterxml.jackson.core.JsonGenerator; |
There was a problem hiding this comment.
Is extending from MdcJsonProvider required? The writeTo interface exposes a jackson class JsonGenerator on the surface, and jackson is in general not recommended because of endless security issues.
There was a problem hiding this comment.
The writeTo method is part of JsonProvider, which is implemented by MdcJsonProvider.
The JsonProvider is used in LogstashEncoder.
I don't think we can change the interface but we can extract jackson library and update to the lastest, what do you think?
There was a problem hiding this comment.
Is there an interface that does not expose jackson on the surface? if not, I think we may need to re-evaluate the feasibility of it. Since adding a new dependency, especially jackson, is a big commitment.
|
|
|
Closed per go/debug-logging-proposal. |
4 participants
In this PR:
b/394880750