Align JSON logs better with ECS by pugnascotia · Pull Request #67266 · elastic/elasticsearch

pugnascotia · 2021-01-11T17:07:25Z

The JSON logs that Elasticsearch produces are roughly in an ECS shape. This PR improves that alignment.

elasticmachine · 2021-01-11T17:07:29Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

yaauie

From my perspective, this looks to address the commentary seen elsewhere. I'm neither an ECS expert nor do I have extensive ES internals knowledge.

pgomulka

it looks great, I left few cosmetic comments

qa/logging-config/src/test/java/org/elasticsearch/common/logging/JsonLoggerTests.java

pgomulka · 2021-01-12T09:39:39Z

server/src/main/java/org/elasticsearch/common/logging/ECSJsonLayout.java

-            };
+            return new KeyValuePair[] {
+                new KeyValuePair("event.dataset", type),
+                new KeyValuePair("elasticsearch.cluster.uuid", "%cluster_id"),


elasticsearch.* are our own defined in ECS fields?
or they are just a custom fields which are not defined there?

Ry told me that the ECS team recommends prefixing Elasticsearch-specific things like cluster and node with elasticsearch.

These fields are not part of ECS but some of them exist in the Filebeat and Metricbeat module: https://www.elastic.co/guide/en/beats/metricbeat/master/metricbeat-module-elasticsearch.html It might be good to align there.

server/src/main/java/org/elasticsearch/common/logging/RateLimitingFilter.java

...ion/qa/rest/src/javaRestTest/java/org/elasticsearch/xpack/deprecation/DeprecationHttpIT.java

pgomulka · 2021-01-12T09:43:26Z

server/src/main/java/org/elasticsearch/common/logging/ECSJsonLayout.java

-                new KeyValuePair("cluster.name","${sys:es.logs.cluster_name}"),
-            };
+            return new KeyValuePair[] {
+                new KeyValuePair("event.dataset", type),


also, do you think it would worthy to change the property used in log4j2.properties file ? from type_name to dataset?
I used type_name only because type is already being used by log4j. With dataset we won't have that problem and I think it would be more clear

I've changed type and type_name to dataset throughout. Do you know whether there's anything breaking about doing that?

ruflin · 2021-01-12T15:54:36Z

@ravi-elastic This change might effect the collection of logs for stack monitoring.

ruflin · 2021-01-12T15:56:07Z

distribution/docker/src/docker/config/oss/log4j2.properties

 appender.rolling.name = rolling
 appender.rolling.layout.type = ECSJsonLayout
-appender.rolling.layout.type_name = server
+appender.rolling.layout.dataset = server


Did not play around with the code itself. But if I think of the dataset, it is a unique name. So it should be elasticsearch.server. Does this make sense?

I'm assuming it follows the logic of the new naming scheme: https://www.elastic.co/blog/an-introduction-to-the-elastic-data-stream-naming-scheme but event.dataset should not be different.

ruflin · 2021-01-12T15:56:59Z

One thing that would be nice to see for review is a log event before and after the change for comparison and also to understand how the final fields look like.

…ing-ecs-fixes

pugnascotia · 2021-01-13T15:51:30Z

@ruflin here we go:

Before:

{
  "@timestamp": "2021-01-13T15:48:16.642Z",
  "cluster.name": "runTask",
  "cluster.uuid": "EV57vhNXQ0iZqPb2pUQfFQ",
  "data_stream.dataset": "deprecation.elasticsearch",
  "data_stream.namespace": "default",
  "data_stream.type": "logs",
  "ecs.version": "1.6",
  "key": "synced_flush",
  "log.level": "DEPRECATION",
  "log.logger": "org.elasticsearch.deprecation.rest.action.admin.indices.RestSyncedFlushAction",
  "message": "Synced flush was removed and a normal flush was performed instead. This transition will be removed in a future version.",
  "node.id": "t1EH7lbISbCZylE9BJ8Z_w",
  "node.name": "runTask-0",
  "process.thread.name": "elasticsearch[runTask-0][transport_worker][T#6]",
  "service.name": "ES_ECS",
  "type": "deprecation"
}

After:

{
  "@timestamp": "2021-01-13T15:44:20.486Z",
  "data_stream.dataset": "elasticsearch.deprecation",
  "data_stream.type": "logs",
  "ecs.version": "1.6",
  "elasticsearch.cluster.name": "runTask",
  "elasticsearch.cluster.uuid": "yjW_iFxzTmGocjNKga2p-Q",
  "elasticsearch.node.id": "wKqe9WlkRIuvUwpuTecrXA",
  "elasticsearch.node.name": "runTask-0",
  "event.code": "synced_flush",
  "event.dataset": "deprecation",
  "log.level": "DEPRECATION",
  "log.logger": "org.elasticsearch.deprecation.rest.action.admin.indices.RestSyncedFlushAction",
  "message": "Synced flush was removed and a normal flush was performed instead. This transition will be removed in a future version.",
  "process.thread.name": "elasticsearch[runTask-0][transport_worker][T#6]",
  "service.name": "ES_ECS"
}

pgomulka · 2021-01-13T16:59:42Z

we tend to update https://github.com/elastic/beats/tree/master/filebeat/module/elasticsearch/deprecation/test also with our sample logs. I think I was usually using some of the tests output. Will try to find this testcase and comment here

ruflin · 2021-01-14T12:28:04Z

@pugnascotia This looks great. Nit: In the second event example data_stream.namespace is missing. Is that on purpose or maybe even a feature because the user can configure it?

…ing-ecs-fixes

pugnascotia · 2021-01-14T14:55:28Z

@ruflin I must have misunderstood a comment elsewhere and thought that data_stream.namespace wasn't necessary, but your last comment implies that it is. It's not configurable, so what value should it have?

We only want to change `ECSJsonLayout`.

ruflin · 2021-01-15T12:42:28Z

If it is not configurable, it should default to default as the value. If the logs are collected by Elastic Agent, I would assume this would be filled in / completed if missing or overwritten if a different namespace should be selected. But default seems like a good choice for now.

…ing-ecs-fixes

The JSON logs that Elasticsearch produces are roughly in an ECS shape. This PR improves that alignment.

Backport / reimplementation of #67266. The JSON logs that Elasticsearch produces are roughly in an ECS shape. This PR improves that alignment. Since `7.x` has it's own version of `EcsJsonLayout`, most of the changes are there, though I also had to backport `ClusterIdConverter` and `NodeIdConverter`.

Align JSON logs better with ECS

def8928

pugnascotia added >enhancement :Core/Infra/Logging Log management and logging utilities v8.0.0 v7.12.0 labels Jan 11, 2021

pugnascotia requested review from pgomulka, ruflin and yaauie January 11, 2021 17:07

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jan 11, 2021

Fixes

ab75b91

yaauie reviewed Jan 12, 2021

View reviewed changes

pgomulka reviewed Jan 12, 2021

View reviewed changes

pugnascotia added 2 commits January 12, 2021 14:06

Rename type_name to dataset in logging classes and resources

cd65dbe

Migrate more log field name locations

7b2a990

ruflin reviewed Jan 12, 2021

View reviewed changes

pugnascotia mentioned this pull request Jan 13, 2021

Introduce deprecation categories #67443

Merged

pugnascotia added 3 commits January 13, 2021 15:34

Checkstyle

ac7cf01

Merge remote-tracking branch 'upstream/master' into deperecation-logg…

5022b33

…ing-ecs-fixes

Test fix

13a3f63

Merge remote-tracking branch 'upstream/master' into deperecation-logg…

67d68b3

…ing-ecs-fixes

Revert ESJsonLayout changes

7b830ff

We only want to change `ECSJsonLayout`.

pugnascotia added 3 commits January 15, 2021 14:03

Add back data_stream.namespace, bump ecs version to 1.7

6ebb8ab

Merge remote-tracking branch 'upstream/master' into deperecation-logg…

b178fde

…ing-ecs-fixes

Merge remote-tracking branch 'upstream/master' into deperecation-logg…

04e3e7e

…ing-ecs-fixes

pugnascotia merged commit c841b2c into elastic:master Jan 25, 2021

pugnascotia deleted the deperecation-logging-ecs-fixes branch January 25, 2021 10:43

pugnascotia added a commit to pugnascotia/elasticsearch that referenced this pull request Jan 25, 2021

Align JSON logs better with ECS (elastic#67266)

e9a734a

The JSON logs that Elasticsearch produces are roughly in an ECS shape. This PR improves that alignment.

pugnascotia mentioned this pull request Jan 25, 2021

Align JSON logs better with ECS #67898

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Conversation

pugnascotia commented Jan 11, 2021

Uh oh!

elasticmachine commented Jan 11, 2021

Uh oh!

yaauie left a comment

Choose a reason for hiding this comment

Uh oh!

pgomulka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pgomulka Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

pugnascotia Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

ruflin Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pgomulka Jan 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pugnascotia Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

ruflin commented Jan 12, 2021

Uh oh!

ruflin Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

ruflin commented Jan 12, 2021

Uh oh!

pugnascotia commented Jan 13, 2021

Uh oh!

pgomulka commented Jan 13, 2021

Uh oh!

ruflin commented Jan 14, 2021

Uh oh!

pugnascotia commented Jan 14, 2021

Uh oh!

ruflin commented Jan 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pgomulka Jan 12, 2021 •

edited

Loading