Skip to content

JSON logging refactoring and X-Opaque-ID support #41354

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

Merged
merged 52 commits into from
Jul 10, 2019

Conversation

pgomulka
Copy link
Contributor

@pgomulka pgomulka commented Apr 18, 2019

This is a refactor to current JSON logging to make it more open for extensions
and support for custom ES log messages used inDeprecationLogger IndexingSlowLog , SearchSLowLog
We want to include x-opaque-id in deprecation logs. The easiest way to have this as an additional JSON field instead of part of the message is to create a custom DeprecatedMessage (extends ESLogMEssage)

These messages are regular log4j messages with a text, but also carry a map of fields which can then populate the log pattern. The logic for this lives in ESJsonLayout and ESMessageFieldConverter.

Similar approach can be used to refactor IndexingSlowLog and SearchSlowLog JSON logs to contain fields previously only present as escaped JSON string in a message field.

closes #41350

@pgomulka pgomulka added WIP :Core/Infra/Logging Log management and logging utilities labels Apr 18, 2019
@pgomulka pgomulka self-assigned this Apr 18, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

System.err.println(wrongLoggerUsage.getErrorLines());
wrongUsageFound[0] = true;
}, args);
// checkLoggerUsage(wrongLoggerUsage -> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still need to figure out how to fix this test

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is still commented out. We should still make it work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absolutely, thanks for reminding!

@pgomulka pgomulka requested a review from rjernst July 2, 2019 21:05
@pgomulka
Copy link
Contributor Author

pgomulka commented Jul 3, 2019

@elasticmachine run elasticsearch-ci/packaging-sample

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. A few final requests

super(fieldMap(xOpaqueId), messagePattern, args);
}

public static Map<String, Object> fieldMap(String xOpaqueId) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be private?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should - fixed

private String esMessageFields;

public Builder() {
super();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this wasn't addressed

@@ -129,7 +130,13 @@ public void close() {
*/
public StoredContext stashContext() {
final ThreadContextStruct context = threadLocal.get();
threadLocal.set(null);
if(context.requestHeaders.containsKey(Task.X_OPAQUE_ID)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we must have this special case, then there should at least be a good explanation in comment on why it is necessary.

minor nit: there are spacing issues in the if and else lines

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added comment and fixed spacing

@pgomulka
Copy link
Contributor Author

pgomulka commented Jul 4, 2019

@elasticmachine update branch

@pgomulka
Copy link
Contributor Author

pgomulka commented Jul 5, 2019

@elasticmachine run elasticsearch-ci/packaging-sample

Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've retested the current state of this PR with elastic/beats#11939 and all still looks good from a Filebeat parsing POV. Thanks!

@pgomulka pgomulka merged commit 03f4e81 into elastic:master Jul 10, 2019
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Jul 10, 2019
This is a refactor to current JSON logging to make it more open for extensions
and support for custom ES log messages used inDeprecationLogger IndexingSlowLog , SearchSLowLog
We want to include x-opaque-id in deprecation logs. The easiest way to have this as an additional JSON field instead of part of the message is to create a custom DeprecatedMessage (extends ESLogMEssage)

These messages are regular log4j messages with a text, but also carry a map of fields which can then populate the log pattern. The logic for this lives in ESJsonLayout and ESMessageFieldConverter.

Similar approach can be used to refactor IndexingSlowLog and SearchSlowLog JSON logs to contain fields previously only present as escaped JSON string in a message field.

closes elastic#41350
pgomulka added a commit that referenced this pull request Jul 12, 2019
…4178)

This is a refactor to current JSON logging to make it more open for extensions
and support for custom ES log messages used inDeprecationLogger IndexingSlowLog , SearchSLowLog
We want to include x-opaque-id in deprecation logs. The easiest way to have this as an additional JSON field instead of part of the message is to create a custom DeprecatedMessage (extends ESLogMEssage)

These messages are regular log4j messages with a text, but also carry a map of fields which can then populate the log pattern. The logic for this lives in ESJsonLayout and ESMessageFieldConverter.

Similar approach can be used to refactor IndexingSlowLog and SearchSlowLog JSON logs to contain fields previously only present as escaped JSON string in a message field.

closes #41350
 backport #41354
pgomulka added a commit that referenced this pull request Jul 19, 2019
By mistake in 7.x types field was removed from slow logs. Types are
still present in that version, so this have to be present as a JSON
field
relates #41354
backport that was causing this #44178
pgomulka added a commit that referenced this pull request Jul 22, 2019
…allowed (#44587)

Deprecation logger was filtering log entries by key, that means that if two log messages with the same key are logged from different users, then the second log messages will be filtered.
This change allows to log deprecation message with the same key by different users.

relates #41354
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Jul 22, 2019
…allowed (elastic#44587)

Deprecation logger was filtering log entries by key, that means that if two log messages with the same key are logged from different users, then the second log messages will be filtered.
This change allows to log deprecation message with the same key by different users.

relates elastic#41354
pgomulka added a commit that referenced this pull request Jul 22, 2019
…allowed backport(#44587) #44682

Deprecation logger was filtering log entries by key, that means that if two log messages with the same key are logged from different users, then the second log messages will be filtered.
This change allows to log deprecation message with the same key by different users.

relates #41354
backport #44587
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

X-Opaque-ID support and JSON logging improvement
7 participants