Add logstash into spring autoconfiguration and provide logstash raw marker config possibility #811
Conversation
pbouillet
requested review from
AlexanderYastrebov,
lukasniemeier-zalando and
whiskeysierra
as code owners
|
Seems like there is new identified vulnerability in the kotlin-stdlib library that is used by logbook-okhttp: https://nvd.nist.gov/vuln/detail/CVE-2020-15824. As this is not related to my PR, I leave it as it is right now. Guess there is a change needed to either ignore or update to a newer version. |
|
Yes, I'll need to deal with this separately.
…On Wed, 12 Aug 2020, 11:06 Patrice Bouillet, ***@***.***> wrote:
Seems like there is new identified vulnerability in the kotlin-stdlib
library that is used by logbook-okhttp:
https://nvd.nist.gov/vuln/detail/CVE-2020-15824. As this is not related
to my PR, I leave it as it is right now. Guess there is a change needed to
either ignore or update to a newer version.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#811 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADI7HICIAFLP6SUVDIRUL3SAJLQZANCNFSM4P4LASCA>
.
|
|
@pbouillet I fixed the CVE issue in master. If you rebase onto the latest version the build error should be gone. I'll take a look at the code now. |
|
@whiskeysierra done, thanks! |
| @@ -17,22 +16,32 @@ | |||
|
|
|||
| import static org.apiguardian.api.API.Status.EXPERIMENTAL; | |||
|
|
|||
| @AllArgsConstructor | |||
There was a problem hiding this comment.
I'd leave that in, so you don't have to implement it by hand.
There was a problem hiding this comment.
Fine for me with your suggestion! Thanks
| @API(status = EXPERIMENTAL) | ||
| public final class LogstashLogbackSink implements Sink { | ||
|
|
||
| private final static Logger log = LoggerFactory.getLogger(Logbook.class); | ||
|
|
||
| private final HttpLogFormatter formatter; | ||
|
|
||
| private String baseField = "http"; |
There was a problem hiding this comment.
| private String baseField = "http"; | |
| private final String fieldName; |
| private String baseField = "http"; | ||
|
|
||
| public LogstashLogbackSink(final HttpLogFormatter formatter) { | ||
| this.formatter = formatter; |
There was a problem hiding this comment.
| this.formatter = formatter; | |
| this(formatter, "http"); |
| public LogstashLogbackSink(final HttpLogFormatter formatter, final String baseField) { | ||
| this.formatter = formatter; | ||
| this.baseField = baseField; | ||
| } |
There was a problem hiding this comment.
| public LogstashLogbackSink(final HttpLogFormatter formatter, final String baseField) { | |
| this.formatter = formatter; | |
| this.baseField = baseField; | |
| } |
logbook-logstash/src/main/java/org/zalando/logbook/logstash/LogstashLogbackSink.java
Show resolved
Hide resolved
logbook-logstash/src/test/java/org/zalando/logbook/logstash/LogbackLogstashSinkTest.java
Show resolved
Hide resolved
| @@ -245,12 +246,22 @@ public Strategy withoutBody() { | |||
| @API(status = INTERNAL) | |||
| @Bean | |||
| @ConditionalOnMissingBean(Sink.class) | |||
| public Sink sink( | |||
| @ConditionalOnProperty(value = "logbook.sink.style", havingValue = "default", matchIfMissing = true) | |||
There was a problem hiding this comment.
I'd move this to a separate PR. Until now the only way to configure Sinks is programmatically, by registering a bean. I have to think about whether we can (and should?) offer that also for other Sink implementations.
There was a problem hiding this comment.
(By this, I'm referring to the whole Spring Boot Auto Configuration support aspect of this PR.)
|
PR has been updated to only focus the change in the logback sink. Let me know if more needs to be changed! |
|
👍 |
|
Thank you for your contribution! 🎉 |
|
I released it as 2.2.0 just now. Should be available in central soon. |
|
Awesome, thank you! Let me know if you need the other part as a PR in the future! |
Description
2 changes overall:
Motivation and Context
The first change is motivated by an issue in our logging environment (ELK). As we use it for many applications, the http field name clashes with an already existing one which is indexed and defined as a different type. Because of this, we cannot even see the requests and responses logged by logbook. For this, I introduced a possibility to change this field name.
The second change is more of a convenient change for Spring autoconfiguration. Currently, we need to define an own Spring bean to be able to use this Logstash sink. I'm not sure if you have some requirements on if and how configuration changes should be for your framework, so feedback is pretty much appreciated (and if needed, I could roll back the second change if you don't feel this is a good way)
I added both changes in a way that the defaults are as before and no breaking changes are introduced.
Types of changes
Checklist: