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

[core.logging] Deprecates legacy logging dest, json, verbosity and rotate configurations #94238

Merged

Conversation

TinaHeiligers
Copy link
Contributor

@TinaHeiligers TinaHeiligers commented Mar 9, 2021

Summary

Resolves #82431.
Closes #86125

This PR deprecates legacy logging configuration in favor of the new KP logging system.
These include:

  • logging.silent
  • logging.quiet
  • logging.verbose
  • logging.dest
  • logging.json
  • logging.rotate & sub-options: enabled, usePolling, pollingInterval, everyBytes, keepFiles
  • logging.events
  • logging.filter

The deprecation warnings shown when legacy logging is configured include a short description about KP logging config alternatives and link to the KP logging README. These links will be changed to reference the user-facing docs once #57502 is addressed.

The deprecations also apply to any configuration passed in as cli arguments except for the short-hand versions of --verbose and --silent logging levels.
--verbose is now handled slightly differently and will do the following:

  • When running in dev mode (env === 'development), deprecation warnings for using --json won't be shown (we explicitly set --logging.json in dev cli).
  • When KP logging is configured, sets KP root log level to all
  • When KP isn't configured, sets legacy log level to verbose (logging.verbose = true)

Release Notes

Deprecates legacy logging configuration in favor of the new Kibana Platform logging system. For example, deprecates logging.json and logging.rotate.*.

--verbose will replace legacy-format logs with Kibana platform log format if logging.root.appenders is configured and won't show a deprecation warning. If Kibana platform logging is not configured, --verbose sets logging.verbose: true and will warn of the deprecated configuration.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@TinaHeiligers TinaHeiligers added release_note:deprecation Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0 Feature:Logging v7.13.0 labels Mar 9, 2021
Copy link
Contributor Author

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

Self review.

src/core/server/config/deprecation/core_deprecations.ts Outdated Show resolved Hide resolved
src/core/server/logging/README.mdx Show resolved Hide resolved
src/core/server/logging/README.mdx Show resolved Hide resolved
src/core/server/logging/README.mdx Show resolved Hide resolved
Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

@TinaHeiligers One thing that occurred to me when summarizing these changes for the beats team is that we might want to mention in the docs that switching to the new logging config is going to mean you start seeing duplicate log entries in both formats.

Most users will have not run into this yet, but if they are ingesting their Kibana logs somewhere, it would be good for them to know that using the new config will result in additional log entries which are in a different structure.

@TinaHeiligers TinaHeiligers force-pushed the logging/deprecate-legacy-config branch from 9639539 to abae294 Compare March 12, 2021 18:51
@elastic elastic deleted a comment from kibanamachine Mar 15, 2021
@elastic elastic deleted a comment from kibanamachine Mar 15, 2021
@TinaHeiligers TinaHeiligers marked this pull request as ready for review March 15, 2021 20:57
@TinaHeiligers TinaHeiligers requested a review from a team as a code owner March 15, 2021 20:57
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

One question I want to raise is whether we should consider adding deprecation notices for the undocumented logging.filter, logging.events.log, and logging.events.error... AFICT those are the only remaining legacy logging configs that would not be deprecated once this PR is merged.

On the one hand, as they were never officially documented, there should be no expectation of them not breaking. But on the other hand, if someone stumbled upon these settings, it would make their lives easier to get a deprecation notice warning them when they are in use. WDYT?

src/core/server/config/deprecation/core_deprecations.ts Outdated Show resolved Hide resolved
src/core/server/config/deprecation/core_deprecations.ts Outdated Show resolved Hide resolved
) {
log(
'"logging.rotate" and sub-options have been deprecated and will be removed in 8.0. ' +
'Moving forward, you can enabled log rotation using the "rolling-file" appender for a context ' +
Copy link
Member

Choose a reason for hiding this comment

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

enabled -> enable is a typo
context -> logger is a nit (In #90764 we switched from labeling loggers with context to just using name... but there are probably a few other places in the logging docs where we'd need to update this, so no strong feelings on it).

Suggested change
'Moving forward, you can enabled log rotation using the "rolling-file" appender for a context ' +
'Moving forward, you can enable log rotation using the "rolling-file" appender for a logger ' +

docs/migration/migrate_8_0.asciidoc Outdated Show resolved Hide resolved
docs/migration/migrate_8_0.asciidoc Show resolved Hide resolved
src/cli/serve/serve.js Outdated Show resolved Hide resolved
src/core/test_helpers/kbn_server.ts Outdated Show resolved Hide resolved
src/legacy/server/logging/index.js Outdated Show resolved Hide resolved
src/legacy/server/kbn_server.js Show resolved Hide resolved
src/cli/serve/serve.js Outdated Show resolved Hide resolved
@TinaHeiligers
Copy link
Contributor Author

TinaHeiligers commented Mar 16, 2021

One question I want to raise is whether we should consider adding deprecation notices for the undocumented logging.filter, logging.events.log, and logging.events.error... AFICT those are the only remaining legacy logging configs that would not be deprecated once this PR is merged.

On the one hand, as they were never officially documented, there should be no expectation of them not breaking. But on the other hand, if someone stumbled upon these settings, it would make their lives easier to get a deprecation notice warning them when they are in use. WDYT?

@lukeelmers This is a tough call. I'm not apposed to helping folks out if we have an alternative in the KP logging system. We don't yet support filtering in the KP so I'd err on the side of not adding a deprecation warning for that. We could add deprecation warnings for logging.events.log and logging.events.error (what's the expected value type for these, i.e. logging.event.error: ????) since we do offer alternatives for those (logging level and all errors are automatically logged or one can explicitly set the root logger level to error).

However, I'm not sure if it would be more of a surprise for folks to start seeing these warnings only to have the settings removed completely in a few minors.

I'll add them and we can see what other reviewers think.

src/legacy/server/logging/index.js Outdated Show resolved Hide resolved
docs/migration/migrate_8_0.asciidoc Outdated Show resolved Hide resolved
packages/kbn-legacy-logging/src/schema.ts Outdated Show resolved Hide resolved
src/cli/serve/serve.js Outdated Show resolved Hide resolved
src/cli/serve/serve.js Outdated Show resolved Hide resolved
src/core/server/config/deprecation/core_deprecations.ts Outdated Show resolved Hide resolved
@joshdover
Copy link
Contributor

This is a tough call. I'm not apposed to helping folks out if we have an alternative in the KP logging system. We don't yet support filtering in the KP so I'd err on the side of not adding a deprecation warning for that. We could add deprecation warnings for logging.events.log and logging.events.error (what's the expected value type for these, i.e. logging.event.error: ????) since we do offer alternatives for those (logging level and all errors are automatically logged or one can explicitly set the root logger level to error).

However, I'm not sure if it would be more of a surprise for folks to start seeing these warnings only to have the settings removed completely in a few minors.

I'll add them and we can see what other reviewers think.

I prefer we show warnings for all settings that will no longer work, even if they were never documented and/or don't have a replacement in 8.0. +1 on the warnings for all 3 of those configs.

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

Looks really close! The only scenario that doesn't seem to be working quite right for me is the one where I have the KP logger configured (logging.root.appenders is set) and I'm using the --verbose flag. It's still showing a warning log message about logging.verbose. I left a comment below about how to fix the issue.

docs/migration/migrate_8_0.asciidoc Show resolved Hide resolved
src/cli/serve/serve.js Outdated Show resolved Hide resolved
Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Updates here LGTM, thanks for adding those logging.events and logging.filter deprecations too!

if (has(settings, 'logging.events.error')) {
log(
'"logging.events.error" has been deprecated and will be removed ' +
'in 8.0. Moving forward, you can use "logging.root.level: error" in your logging configuration. '
Copy link
Member

Choose a reason for hiding this comment

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

super nit: Could still link to the docs for both of the logging.events deprecations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lukeelmers is it ok if we do that once the docs for the KP logging system are added and I go back and change the other links?

Copy link
Member

Choose a reason for hiding this comment

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

works for me 🙂

@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

@TinaHeiligers TinaHeiligers added the auto-backport Deprecated - use backport:version if exact versions are needed label Mar 17, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@TinaHeiligers TinaHeiligers merged commit 28942df into elastic:master Mar 17, 2021
@TinaHeiligers TinaHeiligers deleted the logging/deprecate-legacy-config branch March 17, 2021 23:50
@kibanamachine
Copy link
Contributor

💔 Backport failed

❌ 7.x: Commit could not be cherrypicked due to conflicts

To backport manually, check out the target branch and run:
node scripts/backport --pr 94238

TinaHeiligers added a commit to TinaHeiligers/kibana that referenced this pull request Mar 18, 2021
…tate configurations (elastic#94238)

Co-authored-by: Kibana Machine <[email protected]>
# Conflicts:
#	docs/migration/migrate_8_0.asciidoc
#	src/core/server/config/deprecation/core_deprecations.ts
TinaHeiligers added a commit that referenced this pull request Mar 18, 2021
…tate configurations (#94238) (#94878)

Co-authored-by: Kibana Machine <[email protected]>
# Conflicts:
#	docs/migration/migrate_8_0.asciidoc
#	src/core/server/config/deprecation/core_deprecations.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Logging release_note:deprecation Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.13.0 v8.0.0
Projects
None yet
8 participants