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] Adds a global deprecation warning about log format change. #120306

Merged
merged 2 commits into from
Dec 3, 2021

Conversation

lukeelmers
Copy link
Member

After running into an issue where the RPM package created deprecation warnings in 7.16 due to the way it configured logging, we decided we would skip the deprecation in the RPM package service description.

When we introduced the new logging system in 7.13, we were not worried about awareness of the new logging format, because:

  • folks would need to "opt in" by changing their logging configuration in order to see the new format
  • by explicitly deprecating the old logging configs, we were forcing users to refer to the docs and learn about the new format
  • Upgrade Assistant would make this process easier for users

However, we've now created a scenario where a user could upgrade to 8.0 with zero warning about the new logging format in the Upgrade Assistant (they would only know about this by referring to the breaking changes docs).

So we decided that the safest approach would be to create a deprecation for all users, and register it at the warning level. This way, everyone will be explicitly warned about the change in the Upgrade Assistant, but won't be blocked on upgrading.

Screen Shot 2021-12-02 at 2 13 12 PM

@lukeelmers lukeelmers self-assigned this Dec 2, 2021
@lukeelmers lukeelmers added v7.16.0 v7.16.1 release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting and removed v7.16.0 labels Dec 2, 2021
Comment on lines +25 to +29
expect(messages).toEqual(
expect.arrayContaining([
'Environment variable "CONFIG_PATH" is deprecated. It has been replaced with "KBN_PATH_CONF" pointing to a config folder',
])
);
Copy link
Member Author

Choose a reason for hiding this comment

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

Adding a deprecation that's always registered broke all of our snapshot tests, as they were taking a snapshot of the entire list of deprecation messages. So rather than bloat the snapshots by inlining the new deprecation in every test, I changed the assertions to use expect.arrayContaining instead.

@@ -382,7 +382,7 @@ const quietLoggingDeprecation: ConfigDeprecation = (
message: i18n.translate('core.deprecations.loggingQuiet.deprecationMessage', {
defaultMessage:
'"logging.quiet" 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 Author

Choose a reason for hiding this comment

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

These added spaces were just typos that I caught when converting the snapshot tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

ya, I think my VSCode setup needs some TLC

{ branch }
) => {
addDeprecation({
configPath: 'logging',
Copy link
Member Author

Choose a reason for hiding this comment

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

configPath isn't really relevant here since this is a general warning that is only indirectly related to configuration, but since it is required I set it to logging.

The alternative would be to register this deprecation elsewhere in core (i.e. not as a ConfigDeprecation), but placing it here alongside the other logging deprecations requires fewer changes and keeps things in one place.

defaultMessage: `Learn more about our new logging system by checking out the documentation.`,
}),
i18n.translate('core.deprecations.loggingFormat.manualSteps3', {
defaultMessage: `Learn more about ECS at https://www.elastic.co/guide/en/ecs/8.0/ecs-reference.html.`,
Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't render as a real link in the UA, but I felt like it would still be useful to include...

@lukeelmers lukeelmers marked this pull request as ready for review December 2, 2021 23:07
@lukeelmers lukeelmers added Feature:Upgrade Assistant Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Dec 2, 2021
@elasticmachine
Copy link
Contributor

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

@lukeelmers
Copy link
Member Author

lukeelmers commented Dec 2, 2021

cc @tylersmalley @jbudz @LeeDr @TinaHeiligers Per our slack thread

Also JFYI @cjcenizal

Copy link
Contributor

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

deprecation that's always registered

Does that mean the deprecation warning will show up even if the deployment doesn't configure any logging settings in kibana.yml?
This might annoy folks who haven't explicitly configured any logging settings. That being said, there isn't really an easy way to handle deprecations based on how the stack is stood up, leaving us no choice but to generalize.
It seems like a reasonable compromise to warn everyone about the logging changes but not hold the migration if it's not needed.

@TinaHeiligers TinaHeiligers self-requested a review December 2, 2021 23:59
Copy link
Contributor

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

As agreed on offline, this seems to be the best compromise.
LGTM on green

@lukeelmers
Copy link
Member Author

deprecation that's always registered

Does that mean the deprecation warning will show up even if the deployment doesn't configure any logging settings in kibana.yml?
This might annoy folks who haven't explicitly configured any logging settings.

Yeah, it does mean that the deprecation warning will show for everyone. However, I don't think this warning is really related to how you are configuring logging: no matter what your configuration, whether you are opting in to the new logging system via the config, or still on the legacy config, your log formats will change once you upgrade to 8.0. The impact is lower for folks who are opting in to the new logging config, but there will still be a change for them too (the duplicate log entries in legacy format go away).

So I don't really see a way to limit who we show this warning to. I suppose we could determine if someone is using the new logging config and skip it for them on the assumption they maybe know what they're doing, but I think the safest/conservative approach is just to warn everyone.

@lukeelmers lukeelmers enabled auto-merge (squash) December 3, 2021 16:22
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @lukeelmers

@lukeelmers lukeelmers merged commit 3abdcb8 into elastic:7.16 Dec 3, 2021
@lukeelmers lukeelmers deleted the logging-format-deprecation branch December 3, 2021 17:39
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 Feature:Upgrade Assistant release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.16.1 v7.17.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants