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

Unify Kibana & Elasticsearch logging config keys #57551

Closed
mshustov opened this issue Feb 13, 2020 · 10 comments · Fixed by #93450
Closed

Unify Kibana & Elasticsearch logging config keys #57551

mshustov opened this issue Feb 13, 2020 · 10 comments · Fixed by #93450

Comments

@mshustov
Copy link
Contributor

The config keys naming in Kibana logging config follows the log4j2 conventions https://logging.apache.org/log4j/2.x/manual/configuration.html
While the logging config in elasticsearch follows a bit different convention. https://www.elastic.co/guide/en/elasticsearch/reference/current/logging.html
https://github.com/elastic/elasticsearch/blob/a4a79670f85d5c135c1ad668c387808fffd733f7/qa/logging-config/src/test/resources/org/elasticsearch/common/logging/json_layout/log4j2.properties
Some examples, not full:

Kibana logging config Elasticsearch logging config
logging.appenders.xxx appender.xxx
logging.loggers.xxx logger.xxx
logging.root.xxx rootLogger.xxx
logging.appenders.xxx.kind appender.xxx.type

We can narrow the naming difference as much as possible to reduce the cognitive load on the consumer. We still can have logging-prefix to follow the platform principles, even though the platform is allowed to have exceptions to the rule.

Kibana logging config Kibana config follows ES conventions
logging.appenders.xxx logging.appender.xxx
logging.loggers.xxx logging.logger.xxx
logging.root.xxx logging.root.xxx
logging.appenders.xxx.kind logging.appender.xxx.type
@mshustov mshustov added discuss Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform labels Feb 13, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@pgayvallet
Copy link
Contributor

The Kibana config follows ES conventions you are suggesting looks fine to me.

We still can have logging-prefix to follow the platform principles, even though the platform is allowed to have exceptions to the rule.

I think keeping the prefix really makes sense.

@joshdover
Copy link
Contributor

I like the proposed Kibana settings, but I also think we should have Elasticsearch align with us here as much as possible. Personally, it seems like the appender.xxx config key in ES is very unclear. Adding the logging prefix would make sense to me.

@mshustov mshustov removed the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Feb 26, 2020
@joshdover joshdover mentioned this issue Mar 17, 2020
30 tasks
@tylersmalley tylersmalley added the Team:Operations Team label for Operations Team label May 5, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

@mshustov
Copy link
Contributor Author

mshustov commented Feb 9, 2021

Elasticsearch logging configuration is based on property files.
Kibana configuration is based on yml files.
It's not possible to unify all the keys since log4j uses different configuration keys yml configuration and property file configuration

But we can our best and rename:

  • appender.kind, layout.kind, etc. to appender.type, layout.type
  • file-appender.path to file-appender.fileName
  • logger.xxx.context to logger.xxx.name

the next question whether we want to use Elasticsearch-compatible names for kind/type:

Kibana logging config Elasticsearch-compatible config name
appenders.type: console appenders.type: Console
appenders.type: file appenders.type: File
appenders.type: 'rolling-file' appenders.type: RollingFile
layout.type: json appenders.type: ECSJsonLayout
layout.type: pattern appenders.type: PatternLayout
rolling-file.policy.type: 'size-limit' RollingFile.policy.type: SizeBasedTriggeringPolicy
rolling-file.policy.type: time-interval RollingFile.rolling-file.policy.type: TimeBasedTriggeringPolicy

@joshdover @pgayvallet I'm fine to unify these values as well. WDYT?

@pgayvallet
Copy link
Contributor

But we can our best and rename:

  • appender.kind, layout.kind, etc. to appender.type, layout.type
  • file-appender.path to file-appender.fileName
  • logger.xxx.context to logger.xxx.name

I'm fine with these ones.

the next question whether we want to use Elasticsearch-compatible names for kind/type

That's only my personal opinion, but I've never been a big fan of trying to get too close to ES / log4j exact naming, especially as we are not supporting the exact same features (e.g our PatternLayout does not support the same options as ES's). I'm afraid this could be more misleading for customers than anything else.

Now, I'm aware that this is a common goal to unify configuration among the stack, and I don't have any strong objection, so I'm fine doing it.

@joshdover
Copy link
Contributor

+1 to Pierre's thoughts:

That's only my personal opinion, but I've never been a big fan of trying to get too close to ES / log4j exact naming, especially as we are not supporting the exact same features (e.g our PatternLayout does not support the same options as ES's). I'm afraid this could be more misleading for customers than anything else.

I wonder if it would be confusing if these are the exact same as ES if the options are significantly different. I like the config names themselves being the same, but I think the values should be different if there's a decent amount of divergence in behavior or features. I also feel like the names in Elasticsearch here are really implementation-driven, rather than user-driven.

One caveat: I do like that the JSON layout in ES specifically alludes to its ECS compatibility. What do you all think about renaming layout.type: json to layout.type: ecs-json? It could be more confusing to people who don't know what ECS is, but for those who do (or do quick Google search). If that's the case we could also support an alias for json, ecs, and ecs-json to all mean the same layout.

@mshustov
Copy link
Contributor Author

mshustov commented Mar 3, 2021

I do like that the JSON layout in ES specifically alludes to its ECS compatibility.

I don't think it justifies the effort since we don't support another format for JSON layout: all the JSON logs are ECS-compatible.
And probably not that important for all the customers.
Maybe we can just mention in the docs that the JSON layout is ECS-compatible?

That's only my personal opinion, but I've never been a big fan of trying to get too close to ES / log4j exact naming, especially as we are not supporting the exact same features
+1 to Pierre's thoughts:

Ok, I'm going to close the issue then.

@joshdover
Copy link
Contributor

I don't think it justifies the effort since we don't support another format for JSON layout: all the JSON logs are ECS-compatible.
And probably not that important for all the customers.
Maybe we can just mention in the docs that the JSON layout is ECS-compatible?

Docs are fine, agree it's out of scope for this issue and not super relevant to all customers. Larger scale customers do care more about these logs, so as long we have it documented.

@lukeelmers
Copy link
Member

lukeelmers commented Mar 3, 2021

I don't think it justifies the effort since we don't support another format for JSON layout: all the JSON logs are ECS-compatible.

++ Agreed. Other than the one remaining adjustment I'll make to the LogMeta (#90406), the json logs are already ECS anyway, and TBH I don't really see a huge benefit to offering "non-ECS" formatted JSON. (Esp. as this would be nearly identical to the ECS JSON, with the exception of meta handling)

Larger scale customers do care more about these logs, so as long we have it documented.

I'll make sure docs are updated in #90406

Re: The config naming questions, I'm in agreement with Pierre's comments as well. As long as the functionality is not exactly the same, I think it's okay for the config naming to be close-but-not-identical to avoid potential confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants