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

[8.0] Remove legacy logging #112305

Merged
merged 44 commits into from
Oct 5, 2021

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Sep 15, 2021

Summary

Merge blocked by https://github.com/elastic/cloud/issues/80187

Fix #50660
Fix #82240
Part of #103915

All in title.

Tasks

  • Remove @kbn/legacy-logging
  • Remove usages of LegacyObjectToConfigAdapter
  • Remove @hapi/good / @hapi/good-squeeze / @hapi/podium from kibana dependencies
  • Remove legacy appender src/core/server/legacy/logging/appenders
  • Remove validation that requires logging.root.appenders to have a default
    • this can now be optional and populated with the default value if nothing is explicitly configured
  • Remove legacy service (src/core/server/legacy)
  • Remove deprecated config keys from kibana-docker file
  • Update src/cli/serve/serve.js to remove references to removed deprecated flags (quiet, log-file), and remove references to deprecated legacy config (used inside verbose and silent)
  • Remove legacy logging related config deprecations

Checklist

Release Note

Kibana's logging configuration and log output format has changed. Please refer to the documentation for more detail on how to configure the new logging system.

@pgayvallet pgayvallet added backport:skip This commit does not require backporting Feature:Logging release_note:breaking Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0 labels Sep 16, 2021
@pgayvallet
Copy link
Contributor Author

retest

@tylersmalley
Copy link
Contributor

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

Copy link
Contributor Author

@pgayvallet pgayvallet 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.

I tried to grep all usages of the removed settings in the documentation, but may have potentially missed some, a second pair of eyes would be more than welcome.

@@ -163,7 +162,7 @@ export class CliDevMode {
runExamples: cliArgs.runExamples,
cache: cliArgs.cache,
dist: cliArgs.dist,
quiet: !!cliArgs.quiet,
quiet: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elastic/kibana-operations I kept the property in the Optimizer options and just set it to false to avoid more downstream changes. This can eventually be cleaned up as a follow-up (unless we really want to do it in current PR).

packages/kbn-config/src/config_service.ts Show resolved Hide resolved
@@ -21,8 +21,6 @@ export interface EnvOptions {
export interface CliArgs {
dev: boolean;
envName?: string;
/** @deprecated */
quiet?: boolean;
silent?: boolean;
verbose?: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't sure if I should keep silent and verbose in CliArgs. Those are effectively unused inside core, as now their only usages is to change logging.root.level from src/cli/serve/serve.js, but at the same times, they're still cli args, so we may want to have them propagated to core for telemetry of some other need. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know how many users rely on silent and verbose cli flags? What bothers me is that we allow using them as CLI flags but not as config values. Btw we need to replace all the logging.* values in kibana.yml with their Kibana Logging system counterparts.

Copy link
Member

Choose a reason for hiding this comment

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

Do we know how many users rely on silent and verbose cli flags? What bothers me is that we allow using them as CLI flags but not as config values.

Yeah this bugs me too -- IIRC when we had last discussed this (I think with @spalger ?), the case was made that configuring appenders via the CLI is more cumbersome than when working in the yaml, and that it was a minimal maintenance burden to keep silent and verbose in place, which is why we only deprecated quiet at the time.

src/cli/serve/serve.js Outdated Show resolved Hide resolved
src/core/server/legacy/index.ts Show resolved Hide resolved
src/core/server/logging/logging_config.ts Show resolved Hide resolved
src/core/test_helpers/kbn_server.ts Show resolved Hide resolved
@pgayvallet pgayvallet marked this pull request as ready for review September 23, 2021 10:19
@pgayvallet pgayvallet requested review from a team as code owners September 23, 2021 10:19
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@mshustov mshustov left a comment

Choose a reason for hiding this comment

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

Don't forget to remove the legacy logging system mentions in

And ping Security solution. Detection engine team to update their readme file

src/core/test_helpers/kbn_server.ts Show resolved Hide resolved
logging.rotate.usePolling
logging.silent
logging.useUTC
logging.verbose
Copy link
Contributor

Choose a reason for hiding this comment

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

@elastic/kibana-security is xpack.security.audit.appender.legacyLoggingConfig still necessary in this file?

@@ -21,8 +21,6 @@ export interface EnvOptions {
export interface CliArgs {
dev: boolean;
envName?: string;
/** @deprecated */
quiet?: boolean;
silent?: boolean;
verbose?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know how many users rely on silent and verbose cli flags? What bothers me is that we allow using them as CLI flags but not as config values. Btw we need to replace all the logging.* values in kibana.yml with their Kibana Logging system counterparts.

Copy link
Contributor

@jportner jportner left a comment

Choose a reason for hiding this comment

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

Code review only; security plugin unit test snapshot change LGTM 👍

Copy link
Member

@mistic mistic left a comment

Choose a reason for hiding this comment

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

Changes on code under operations team code owners LGTM

@pgayvallet pgayvallet mentioned this pull request Sep 29, 2021
1 task
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
core 30 29 -1

History

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

@pgayvallet pgayvallet merged commit a4b74bd into elastic:master Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Logging release_note:breaking Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0
Projects
None yet
9 participants