Skip to content
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

Introduce logging JsonLayout. #13180

Merged

Conversation

azasypkin
Copy link
Member

@azasypkin azasypkin commented Jul 28, 2017

In this PR we introduce new JsonLayout that can be used by logging appenders to output JSON strings instead of human-readable log messages.

Fixes #12941

Depends and based on #12852

Can be tested with the following config example:

logging.appenders.default:
  kind: console
  layout.kind: json

@azasypkin azasypkin force-pushed the issue-12941-logging-json-output branch from 3f18ff7 to 2125cff Compare July 31, 2017 08:25
format(record: LogRecord): string {
// Per spec `timestamp.toJSON()` uses `timestamp.toISOString()` under the hood.
// See http://www.ecma-international.org/ecma-262/5.1/#sec-15.9.5.44
return JSON.stringify({
Copy link
Member Author

Choose a reason for hiding this comment

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

Output looks like this:

{"timestamp":"2017-07-31T10:39:40.411Z","level":{"id":"info","value":5},"context":"elasticsearch.client.admin","message":"cluster client stopped"}

But I don't know use case for JSON output, really (cc @epixa, @tylersmalley). Maybe we need something like this instead:

format({ timestamp, level, context, message, error, meta }: LogRecord): string {
    // Per spec `timestamp.toJSON()` uses `timestamp.toISOString()` under the hood.
    // See http://www.ecma-international.org/ecma-262/5.1/#sec-15.9.5.44
    return JSON.stringify({
      '@timestamp': timestamp,
      level: level.id,
      context,
      message,
      error: error && error.message,
      meta
    });
  }
{"@timestamp":"2017-07-31T10:35:49.230Z","level":"info","context":"elasticsearch.client.admin","message":"cluster client stopped"}

Copy link
Contributor

Choose a reason for hiding this comment

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

++ I prefer the "more specific" format. We should also take a look at current Kibana to make sure we're "fairly consistent", but I'm good with doing that in a follow-up if you want to

Copy link
Member Author

@azasypkin azasypkin Jul 31, 2017

Choose a reason for hiding this comment

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

Yeah, will make it closer to what we have now in Kibana (it will be trivial to tweak it later anyway):

{  
   "type":"log",
   "@timestamp":"2017-07-31T12:24:32Z",
   "tags":[  
      "warning",
      "elasticsearch",
      "admin"
   ],
   "pid":32107,
   "message":"Unable to revive connection: http://localhost:9200/"
}

and here is what we have in Elasticsearch:

{
  "timeMillis" : 1501510600593,
  "thread" : "Thread-2",
  "level" : "INFO",
  "loggerName" : "org.elasticsearch.node.Node",
  "marker" : {
    "name" : "[SRbYZv9] "
  },
  "message" : "closed",
  "endOfBatch" : false,
  "loggerFqcn" : "org.apache.logging.log4j.spi.AbstractLogger",
  "threadId" : 15,
  "threadPriority" : 5
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Filled dedicated issue to discuss what additional data we should log, especially relevant for JSONLayout: #13241

Copy link
Contributor

@kimjoar kimjoar left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -147,10 +173,9 @@ And assuming you're using `console` appender and `trace` level for `server` cont
[2017-07-25T18:54:41.639Z][DEBUG][server.http] Message with `debug` log level.
```

Obviously your log will be less verbose with `warn` level for the `server` context:
Obviously log will be less verbose with `warn` level for the `server` context:
Copy link
Contributor

Choose a reason for hiding this comment

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

Obviously -> The?

format(record: LogRecord): string {
// Per spec `timestamp.toJSON()` uses `timestamp.toISOString()` under the hood.
// See http://www.ecma-international.org/ecma-262/5.1/#sec-15.9.5.44
return JSON.stringify({
Copy link
Contributor

Choose a reason for hiding this comment

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

++ I prefer the "more specific" format. We should also take a look at current Kibana to make sure we're "fairly consistent", but I'm good with doing that in a follow-up if you want to

@azasypkin
Copy link
Member Author

Merging now, we'll be able to tune format later in the scope of #13241.

@azasypkin azasypkin merged commit cb222d7 into elastic:new-platform Aug 3, 2017
@azasypkin azasypkin deleted the issue-12941-logging-json-output branch August 3, 2017 09:42
@kimjoar kimjoar added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Nov 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants