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

Logging service uses new config format #41956

Closed
17 tasks done
mshustov opened this issue Jul 25, 2019 · 5 comments
Closed
17 tasks done

Logging service uses new config format #41956

mshustov opened this issue Jul 25, 2019 · 5 comments
Assignees
Labels
blocker Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@mshustov
Copy link
Contributor

mshustov commented Jul 25, 2019

Logging in New platform was completely re-written to be compatible with Elasticsearch logging framework and uses a different configuration format.

Switching to a new config is a breaking change and we have to organize the process in several steps:

  • make new config available
  • deprecate legacy config
  • start using new config on the master branch

Subtasks:

Done

#56480

  • start consuming logging config in the NP, fallback to LP via legacy-appender appender
  • setup integration tests
  • provide compatibility with LP settings verbose, silent, quiet
  • provide pid in logs

#56982

  • provide compatibility for meta
  • provide compatibility with LP settings timezone

#57433

Created as followups

@mshustov mshustov added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform labels Jul 25, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@mshustov
Copy link
Contributor Author

mshustov commented Feb 5, 2020

Log Record Format

Level

  • platform: INFO
  • Legacy platform: info

Proposal: document as expected behavior, compatible with elasticsearch logging format.

Tags

  • Platform:'a.b'. a tag is a part of context.
  • Legacy platform: [a][b]. a tag handled a separate entity.

Proposal: document as expected behavior, compatible with elasticsearch logging format.
To note: it’s called component in log4j terms.

PID

  • Platform: N/A
  • Legacy platform output with json: false: NA
  • Legacy platform output with json: true: the current PID

Proposal: Elasticsearch uses log4j which uses Mapped Diagnostic Context providing such info as pid, jvmVersion, OS, etc. Probably that's too much for us at the moment
For kind: 'pattern' add{pid} parameter, but not use it in the default template.
For kind: 'json' add pid property.

Time
  • Platform: YYYY-MM-DDTHH:MM:SS.nnnZ
  • Legacy platform: HH:MM:SS.nnn + the legacy platform allows specifying timezone to convert time.

log4j allows to specify date format for kind: 'pattern' https://logging.apache.org/log4j/log4j-2.2/manual/layouts.html and provides a set of predefined formats. But JSON layout kind: 'json' always outputs timestamp in UNIX milliseconds. To make it more user-friendly we can follow ESJsonLayout setup that uses yyyy-MM-dd'T'HH:mm:ss,SSSZZ date format. https://github.com/elastic/elasticsearch/blob/a4a79670f85d5c135c1ad668c387808fffd733f7/server/src/main/java/org/elasticsearch/common/logging/ESJsonLayout.java#L91
Or use ISO8601 format, since it's the standard.

Proposal:
For kind: 'pattern’: If we want to handle all log4j date formats it would require a lot of work. I'd suggest allowing users to specify a date format supported by moment.js (like yyyy-MM-dd'T'HH:mm:ss,SSSZZ), as a bonus we can ship several pre-defined formats:

  • ISO8601 : 2020-02-01T10:00:00.000
  • ISO801_WITH_TZ: 2020-02-01T10:00:00.000+01:00
  • ABSOLUTE: 10:00:00.000
  • UNIX: 1580899135
  • UNIX_MILLIS: 1580899154504

declaration can be done as date{format}{timezone}

kind: ‘json’: use ESJsonLayout default format yyyy-MM-dd'T'HH:mm:ss,SSSZZ. users cannot specify a different format.

With settings:

meta
  • Legacy platform output with json: false: N/A
  • Legacy platform output with json: true: inlined in body
  • Legacy platform output with json: true and error: N/A
legacyPlatformLogger.info('info', {
 from: 'v7',
 to: 'v8',
});

Output

{
 "from": "v7",
 "to": "v8",
 //..
}
  • Platform output with kind: 'pattern': NA
  • Platform output with kind: 'json':
platformLogger.info('info', {
 from: 'v7',
 to: 'v8',
});

Output

{
 "meta": {
    "from": "v7",
    "to": "v8",
 },
 //..
}

Proposal:
For kind: 'pattern’: support meta property. it can be attached as [{"from": "v7", "to": "v8"}] to make it closer to the well-known JSON format. Note: It doesn't match to the Elasticsearch format with = separator:

userData[{history_uuid=3fkDHdWkTlmaiDMTCT24Ew, local_checkpoint=-1, max_seq_no=-1, max_unsafe_auto_id_timestamp=-1, translog_generation=1, translog_uuid=P7kbJPoeRFK4ovU5DocU_w}]}]

Log4j supports logging a map structure via map pattern https://logging.apache.org/log4j/log4j-2.2/manual/layouts.html It allows users to specify separator, can be done by request later.
For kind: 'json'. support meta with attaching meta into the json body as elasticsearch does:
https://github.com/elastic/elasticsearch/blob/0260c6f55c4623f1195341bdc4e61a7ec7bc1754/qa/logging-config/src/test/java/org/elasticsearch/common/logging/JsonLoggerTests.java#L193-L213
This could lead to overlapping of data.

Pattern format

The current format is not compatible with log4j syntax since uses curly braces to define parameters. To make it compatible we can switch the parameter declaration to %{name}. The main downside that it creates an expectation that our setup supports all log4j rules. We can state in the logging manual that Kibana implements just a sub-set of log4j capabilities and tries to be as close as possible. An alternative is to use the current format that we can customize to our needs.

// current format
{timestamp}|{level}|{message}
// current format with timestamp format
{timestamp{HH:MM:ss.SSS}}|{level}|{message}
{timestamp{HH:MM:ss.SSS}{America/Los_Angeles}}|{level}|{message}
// log4j format
%date{HH:MM:ss.SSS}|%level|%message

Request, response, ops data

Formatting Request, response, ops data should be considered separately.

@pgayvallet
Copy link
Contributor

Level / Tags

I agree, documenting new behavior seems sufficient.

PID
Proposal: Elasticsearch uses log4j which uses Mapped Diagnostic Context providing such info as pid, jvmVersion, OS, etc. Probably that's too much for us at the moment
For kind: 'pattern' add{pid} parameter, but not use it in the default template.
For kind: 'json' add pid property.

I agree that implementing an equivalent to l4j's MDC and NDC is overkill. I'm ok with the proposal

Time

Using ES layout as the default makes sense. Using moment formats seems sufficient.

meta

I don't even know what this should be used for, but handling it only in pattern sounds alright.

meta - It doesn't match to the Elasticsearch format we = separator:

Not sure if this is really an issue, or who could say otherwise.

Pattern format

No opinion on that. From the JS world, curly makes more sense than %{prop}. Also as you said The main downside that it creates an expectation that our setup supports all log4j rules. If we choose to change to %{} we need to document explicitly that we are NOT using l4j

@mshustov
Copy link
Contributor Author

mshustov commented Feb 5, 2020

declaration can be done as date{format}{timezone}

An alternative solution, which is easier to implement, but less flexible and different from log4j format is to declare time format and timezone as a part of the layout config.

layout
  highlight: false
  kind: pattern
  timestamp:
    format: yyyy-MM-dd'T'HH:mm:ss,SSSZZ
    timezone: America/Los_Angeles

@joshdover
Copy link
Contributor

From the JS world, curly makes more sense than %{prop}. Also as you said The main downside that it creates an expectation that our setup supports all log4j rules. If we choose to change to %{} we need to document explicitly that we are NOT using l4j

I'm not sure that we should let the language that Kibana is implemented in drive the way our users use and configure the product itself, at least when we can avoid it.

Personally, I think if we're going to get closer to Elasticsearch/log4j's configuration, we should try to make it as close as we can while being practical. I think it's more confusing if the configuration format is 80% similar than if it's 20% similar. Humans tend to pattern match and will assume it's the same pretty quickly.

That doesn't mean we need to have full feature parity by any means, but I think that for the features we do choose to implement, we should make them work as closely to our other products as possible OR make it clear very different.

Given that premise, my preference is that we implement the %date{HH:MM:ss.SSS}|%level|%message syntax if it's practical enough to do. If the %date{HH:MM:ss.SSS} is difficult, the YAML configuration option would also work. I just don't think the current {timestamp} syntax is a good option.

One thing that might making parsing this simpler is to write a simple grammar using PEG.js, similar to how we parse expressions in @kbn/interpreter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

4 participants