-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Refactor ESLogMessage to not define fields upfront #46702
Refactor ESLogMessage to not define fields upfront #46702
Conversation
Pinging @elastic/es-core-infra |
@pgomulka test |
@elasticmachine update branch |
…asticsearch into feature/logging-parametrised
@@ -72,4 +81,9 @@ public static String asJsonArray(Stream<String> stream) { | |||
.map(ESLogMessage::inQuotes) | |||
.collect(Collectors.joining(", ")) + "]"; | |||
} | |||
|
|||
public static ESLogMessage message(String messagePattern, Object... args){ |
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.
Since we eventually collect the arguments using .with(
we could probably structure this in a way that avoids repeating them. It's probably worth it even if we end up using a builder.
I'm thinking something similar to: https://github.com/logstash/logstash-logback-encoder#event-specific-custom-fields
@@ -72,4 +81,9 @@ public static String asJsonArray(Stream<String> stream) { | |||
.map(ESLogMessage::inQuotes) | |||
.collect(Collectors.joining(", ")) + "]"; | |||
} | |||
|
|||
public static ESLogMessage message(String messagePattern, Object... args){ |
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.
We have tooling to check that loggers are used correctly which I think is now circumvent-able with this method.
Most probably the tooling will have to be updated regardless of how this API is implemented.
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.
you probably mean ESLoggerUsageTests
? I think it should be fine as it was already extended for subclass usages. But I will have a look to make sure it passes fine
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.
Yes I do mean the ESLoggerUsageChecker
I don't know enough about it's implementation to be able to tell if it will work with the builders or not.
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.
@pgomulka Could we maybe add an explicit test to ESLoggerUsageChecker
that uses ESLogMessage.of
just to be clear that this works ?
qa/logging-config/src/test/java/org/elasticsearch/common/logging/JsonLoggerTests.java
Show resolved
Hide resolved
@elasticmachine update branch |
@elasticmachine run elasticsearch-ci/2 |
server/src/main/java/org/elasticsearch/common/logging/DeprecatedMessage.java
Show resolved
Hide resolved
sb.append(Chars.DQUOTE).append(':').append(Chars.DQUOTE); | ||
start = sb.length(); | ||
sb.append(getIndexedReadOnlyStringMap().getValueAt(i).toString()); | ||
// ParameterFormatter.recursiveDeepToString(getIndexedReadOnlyStringMap().getValueAt(i), sb, null); |
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.
should we remove this commented code ?
…asticsearch into feature/logging-parametrised
@elasticmachine run elasticsearch-ci/2 |
@elasticmachine run elasticsearch-ci/packaging-sample-matrix |
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.
LGTM
@elasticmachine run elasticsearch-ci/2 |
@elasticmachine run elasticsearch-ci/packaging-sample |
Slow log's routing value when null was causing ESLogMessage.asJson method to throw Null Pointer Exception. This should be fixed in ESLogMessage as well as prevent passing that key at all. This only happens in 8 because of previous refactoring #46702
The current behaviour expect all additional fields to be defined in ESLogMessage. This makes adding new custom ESLogMessages harder.
This refactoring is changing this behaviour to only define fields in a properties file when overriding a
standard
field for instancemessage
. All other custom fields do not need to be defined upfront in property file.As an example an
AllocationService
log statementCluster health status changed from [{}] to [{}] (reason: [{}]
will have additional 3 fields in json logs.relates #46119