[logging] Adds a global deprecation warning about log format change. #120306
Conversation
lukeelmers
added
v7.16.0
v7.16.1
release_note:skip
| expect(messages).toEqual( | ||
| expect.arrayContaining([ | ||
| 'Environment variable "CONFIG_PATH" is deprecated. It has been replaced with "KBN_PATH_CONF" pointing to a config folder', | ||
| ]) | ||
| ); |
There was a problem hiding this comment.
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. ', | |||
There was a problem hiding this comment.
These added spaces were just typos that I caught when converting the snapshot tests.
There was a problem hiding this comment.
ya, I think my VSCode setup needs some TLC
| { branch } | ||
| ) => { | ||
| addDeprecation({ | ||
| configPath: 'logging', |
There was a problem hiding this comment.
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.`, |
There was a problem hiding this comment.
This doesn't render as a real link in the UA, but I felt like it would still be useful to include...
lukeelmers
added
Feature:Upgrade Assistant
Team:Core
|
Pinging @elastic/kibana-core (Team:Core) |
|
cc @tylersmalley @jbudz @LeeDr @TinaHeiligers Per our slack thread Also JFYI @cjcenizal |
There was a problem hiding this comment.
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.
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. |
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @lukeelmers |
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:
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
warninglevel. This way, everyone will be explicitly warned about the change in the Upgrade Assistant, but won't be blocked on upgrading.