Start consuming np logging config #56480

mshustov · 2020-01-31T11:20:07Z

Summary

The platform starts handling the new logging config.
Removed receiveAllAppenders flag, since it breaks compatibility with the legacy config settings: silent & quiet.
Added based integration tests. I will add more to make sure compatibility (when possible) with the legacy log record format.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~[ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

Dev docs

Provides experimental support of new logging format for new platform plugins. More about logging format: https://github.com/elastic/kibana/blob/master/src/core/server/logging/README.md

it breaks legacy-appender compatibility with legacy flags: silent, quiet, verbose

elasticmachine · 2020-01-31T11:20:10Z

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

mshustov · 2020-01-31T13:45:19Z

src/core/server/legacy/integration_tests/logging.test.ts

+
+ expect(getPlatformLogsFromMock(mockConsoleLog)).toMatchInlineSnapshot(`
+ Array [
+ "[xxxx-xx-xxTxx:xx:xx.xxxZ][INFO ][test-file] info",


We can stripe timestamps. I decided to keep it to see the difference in the format until we have separate tests for them.

mshustov · 2020-01-31T13:47:46Z

src/core/server/legacy/config/legacy_object_to_config_adapter.ts

 const loggingConfig = {
 appenders: {
- default: { kind: 'legacy-appender', legacyLoggingConfig: configValue },
+ ...appenders,
+ default: { kind: 'legacy-appender', legacyLoggingConfig },


rewrites a default appender

kibana/src/core/server/logging/logging_config.ts

Line 155 in e6bbae4

this.appenders.set(appenderKey, appenderSchema);

mshustov · 2020-02-03T14:53:24Z

src/core/server/legacy/config/legacy_object_to_config_adapter.ts

 },
- root: { level: 'info' },
+ root: { level: 'info', ...root },


Note: We allow users to configure roots. Although if a user specifies root.appenders without defaults NP logging service won't send logs to the legacy system. I think logging a warning is enough because I don't want to extend user-defined config value, nor enforce default value presence in root.appenders(since we will remove this logic in v8). WDYT?

Logging a warning makes sense to me 👍

added config validation to prevent server start

joshdover

Tested locally, worked as expected.

src/core/server/logging/logger.ts

pgayvallet

LGTM. My only concern is that during the transition period, when NP logger/appender are configured, there will be formatting differences between messages logged to the 'legacy' console and messages logged with the NP loggers. But that probably can't be avoided.

mshustov · 2020-02-04T08:00:51Z

My only concern is that during the transition period, when NP logger/appender are configured, there will be formatting differences between messages logged to the 'legacy' console and messages logged with the NP loggers. But that probably can't be avoided.

Yes, I'm about to start working on record format. TBH I don't think it's a bit problem because if you want to use the new config format you kinda expect that output has a different form.
Also, I'd like to avoid the long-lived branch that we merge once and realize that something broken. But with the current PR, we already can start testing logging functionality for migrated plugins.

required during migration period to make sure logs are sent to the LP logging system

kibanamachine · 2020-02-04T10:31:46Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 8f5f392

History

💚 Build #23713 succeeded bae5b6f
💔 Build #23700 failed 003cd43

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

* pass config to the new platform * add default appenders * remove receiveAllAppenders flag it breaks legacy-appender compatibility with legacy flags: silent, quiet, verbose * add integration tests * use console mocks to simplify test setup * update tests * improve names * validate that default appender always presents on root level required during migration period to make sure logs are sent to the LP logging system * do not check condition in the loop * fix integration tests

* master: (42 commits) Move kuery_autocomplete ⇒ NP (elastic#56607) [ML] Functional tests - stabilize job row and analytics result view assertions (elastic#56595) [Discover] Inline angular directives only used in this plugin (elastic#56119) [Discover] Migrate get_sort.js test from mocha to TypeScript (elastic#56011) [SIEM] Enable flow_target_select_connected unit tests (elastic#55618) Start consuming np logging config (elastic#56480) [SIEM] Add eslint-plugin-react-perf (elastic#55960) Mention changed SAML ACS endpoint URL in breaking changes doc. (elastic#56613) Add `getServerInfo` API to http setup contract (elastic#56636) Updates Monitoring alert Jest snapshots Kibana property config migrations (elastic#55937) Vislib replacement toggle (elastic#56439) [Uptime] Add unit tests for QueryContext time calculation (elastic#56671) [SIEM][Detection Engine] Critical blocker, fixes pre-packaged rule miscounts Upgrade EUI to v18.3.0 (elastic#56228) [Maps] Fix server log (elastic#56679) [SIEM] Fixes FTUE when APM node is present (elastic#56574) [Reporting/FieldFormats] expose `setFieldFormats` and call from ReportingPlugin.start (elastic#56563) Update EMS API urls for production (elastic#56657) Ability to delete alerts even when AAD is out of sync (elastic#56543) ...

* pass config to the new platform * add default appenders * remove receiveAllAppenders flag it breaks legacy-appender compatibility with legacy flags: silent, quiet, verbose * add integration tests * use console mocks to simplify test setup * update tests * improve names * validate that default appender always presents on root level required during migration period to make sure logs are sent to the LP logging system * do not check condition in the loop * fix integration tests

mshustov added 4 commits January 31, 2020 11:55

pass config to the new platform

9374878

add default appenders

a51c3b4

remove receiveAllAppenders flag

de25412

it breaks legacy-appender compatibility with legacy flags: silent, quiet, verbose

add integration tests

003cd43

mshustov added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.7.0 labels Jan 31, 2020

mshustov requested a review from a team as a code owner January 31, 2020 11:20

mshustov added the WIP Work in progress label Jan 31, 2020

mshustov added 3 commits January 31, 2020 14:21

use console mocks to simplify test setup

7f51269

update tests

e6bbae4

improve names

bae5b6f

mshustov commented Jan 31, 2020

View reviewed changes

mshustov removed the WIP Work in progress label Jan 31, 2020

mshustov mentioned this pull request Jan 31, 2020

Logging service uses new config format #41956

Closed

17 tasks

mshustov commented Feb 3, 2020

View reviewed changes

joshdover approved these changes Feb 3, 2020

View reviewed changes

src/core/server/logging/logger.ts Outdated Show resolved Hide resolved

pgayvallet approved these changes Feb 4, 2020

View reviewed changes

mshustov added 4 commits February 4, 2020 09:01

Merge branch 'master' into start-consuming-np-logging-config

e533d54

validate that default appender always presents on root level

b4b698c

required during migration period to make sure logs are sent to the LP logging system

do not check condition in the loop

554f26f

fix integration tests

8f5f392

mshustov added the v8.0.0 label Feb 4, 2020

mshustov merged commit a36ec32 into elastic:master Feb 4, 2020

mshustov deleted the start-consuming-np-logging-config branch February 4, 2020 11:38

mshustov mentioned this pull request Feb 4, 2020

[7.x] Start consuming np logging config (#56480) #56735

Merged

mshustov added the backported label Feb 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start consuming np logging config #56480

Start consuming np logging config #56480

mshustov commented Jan 31, 2020

elasticmachine commented Jan 31, 2020

mshustov Jan 31, 2020

mshustov Jan 31, 2020

mshustov Feb 3, 2020 •

edited

Loading

joshdover Feb 3, 2020

mshustov Feb 4, 2020

joshdover left a comment

pgayvallet left a comment

mshustov commented Feb 4, 2020

kibanamachine commented Feb 4, 2020

Start consuming np logging config #56480

Start consuming np logging config #56480

Conversation

mshustov commented Jan 31, 2020

Summary

Checklist

For maintainers

Dev docs

elasticmachine commented Jan 31, 2020

mshustov Jan 31, 2020

Choose a reason for hiding this comment

mshustov Jan 31, 2020

Choose a reason for hiding this comment

mshustov Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

joshdover Feb 3, 2020

Choose a reason for hiding this comment

mshustov Feb 4, 2020

Choose a reason for hiding this comment

joshdover left a comment

Choose a reason for hiding this comment

pgayvallet left a comment

Choose a reason for hiding this comment

mshustov commented Feb 4, 2020

kibanamachine commented Feb 4, 2020

💚 Build Succeeded

History

mshustov Feb 3, 2020 •

edited

Loading