[ISSUE-2116] - Add Spring Boot native ECS logging support #2184
Conversation
janar-rahumeel
requested review from
ChristianLohmann,
kasmarian,
lukasniemeier-zalando and
msdousti
as code owners
|
|
||
| @Slf4j | ||
| @RequiredArgsConstructor | ||
| public class DefaultEcsStructuredHttpLogFormatterSupport implements StructuredHttpLogFormatterSupport { |
There was a problem hiding this comment.
Default class for populating ECS fields. Currently only standard ECS fields are used (note headers missing)
| return String.format("Request to %s", ((Map<String, Object>) members.get("url")).get("full")); | ||
| } | ||
|
|
||
| return "null"; // TODO |
| <jmh.version>1.37</jmh.version> | ||
| <lombok.version>1.18.38</lombok.version> | ||
| <logback-classic.version>1.5.18</logback-classic.version> | ||
| <logback-core.version>1.5.8</logback-core.version> |
There was a problem hiding this comment.
Logback core is transitive dependency of Logback classic, hence removed
| <dependency> | ||
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-autoconfigure</artifactId> | ||
| <version>${spring-boot.version}</version> |
| @API(status = INTERNAL) | ||
| @Bean | ||
| @Conditional(ConditionalOnNativeEcsStructuredLoggingFormat.class) | ||
| @ConditionalOnMissingBean(StructuredHttpLogFormatterSupport.class) |
There was a problem hiding this comment.
Custom StructuredHttpLogFormatterSupport can be provided to extend default ECS field set
|
|
||
| @API(status = INTERNAL) | ||
| @Bean | ||
| @Conditional(ConditionalOnNativeEcsStructuredLoggingFormat.class) |
There was a problem hiding this comment.
We only enable the feature when either console or file structured logging is enabled
| @API(status = INTERNAL) | ||
| @Bean | ||
| @Primary | ||
| @Conditional(ConditionalOnNativeEcsStructuredLoggingFormat.class) |
There was a problem hiding this comment.
We only enable the feature when either console or file structured logging is enabled
| @ActiveProfiles | ||
| @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT, classes = LogbookWebTest.Application.class) | ||
| @TestPropertySource | ||
| @interface LogbookWebTest { |
There was a problem hiding this comment.
Annotation to run tests with web application context
|
|
||
| import java.util.function.Consumer; | ||
|
|
||
| public class NativeEcsLoggingExtension implements BeforeAllCallback, AfterAllCallback { |
There was a problem hiding this comment.
This extension helps to reset logging system between tests to properly test built-in structured logging
| } | ||
|
|
||
| @TestConfiguration | ||
| static class Configuration { |
There was a problem hiding this comment.
I test local/remote within the same test
janar-rahumeel
force-pushed
the
issue_2116/add_support_for_ecs_logging
branch
from
11a27bd to
f66358e
Compare
janar-rahumeel
force-pushed
the
issue_2116/add_support_for_ecs_logging
branch
from
f66358e to
d7f3f15
Compare
kasmarian
left a comment
kasmarian
left a comment
There was a problem hiding this comment.
@janar-rahumeel thank you for the contribution! And apologies for not reviewing it for some time. I've left a few comments, some of them I find crucial to address.
p.s. We're also preparing the 4.0.0 release soon, please pull the latest main branch, it has a few breaking changes.
| @@ -1 +1,4 @@ | |||
| org.springframework.boot.autoconfigure.EnableAutoConfiguration = org.zalando.logbook.autoconfigure.LogbookAutoConfiguration | |||
|
|
|||
| org.springframework.boot.logging.structured.StructuredLoggingJsonMembersCustomizer=\ | |||
There was a problem hiding this comment.
A bit of overarching comment: I wonder if it would be better to encapsulate all the ECS classes in a new module instead. E.g. logbook-spring-boot-ecs.
Then autoconfigure module would only declare the HttpLogFormatter bean and clients who need the ecs support, would add the second dependency. Could be even done purely based on the ConditionalOnNativeEcsStructuredLoggingFormat condition even, as it doesn't require any additional dependency.
| import java.io.IOException; | ||
| import java.util.Map; | ||
|
|
||
| public interface StructuredHttpLogFormatterSupport { |
There was a problem hiding this comment.
What's the benefit of having this additional abstraction? Wouldn't it be sufficient to have the relevant logic directly in NativeEcsStructuredHttpLogFormatter ?
| @RequiredArgsConstructor | ||
| public class DefaultEcsStructuredHttpLogFormatterSupport implements StructuredHttpLogFormatterSupport { | ||
|
|
||
| private final StructuredHttpLogFormatter structuredHttpLogFormatter = content -> null; |
There was a problem hiding this comment.
Seems like you're using this object purely because of default methods of the interface. Maybe again to the point of my comment above - why not just stick to a single implementation of the StructuredHttpLogFormatter for ECS instead?
|
|
||
| @Override | ||
| public void customize(JsonWriter.Members<Object> members) { | ||
| members.addMapEntries(ignored -> new HashMap<>(NativeEcsStructuredHttpLogFormatter.ECS_STRUCTURED_MEMBERS.get())); |
There was a problem hiding this comment.
This is my biggest concern for the PR.
- Using a ThreadLocal would be a fragile approach in my view, it won't scale for async/reactive use cases. Where a single request can be handled by multiple threads. The ThreadLocal variable will likely be empty or, worse, contain data from a different request that was previously processed on that thread.
- Even in synchronous world, if ThreadLocal cleanup is missing (which is the case here), stale data could appear in logs of the same thread.
I wonder if a request scoped bean could be utilized here instead of a ThreadLocal or MDC.
2 participants
Description
The main idea is to use StructuredLoggingJsonMembersCustomizer to extend logging context. Please note that this functionality works only for Spring Boot's built-in structured logging
Motivation and Context
Add support for ECS logging via Spring Boot built-in structured logging
Types of changes
Checklist: