Log deprecation warnings for plugins which won't be disable-able in 8.0 #112602

lukeelmers · 2021-09-20T18:56:21Z

This PR is a replacement of #111890 -- apologies if you are being pinged for a second review.

Notes if you are pinged for a CODEOWNERS review

This PR moves forward with the plan outlined in #89584 to remove the ability for plugins to be disable-able by default in 8.0.

If you are tagged for a review here, it is for one of the following reasons:

Your plugin currently has an enabled property in your config schema, so a deprecation has been added for it.
Your plugin currently implicitly has an enabled property but does not have a config schema, however you had indicated you need to preserve the ability to disable it (e.g. vis_type_* plugins). In this case an explicit config has been added for you.

The good news is that most likely you'll only need to review one or two lines of code 🙂

To test the deprecations:

Start Kibana with a default/empty logging config, and <my-plugin>.enabled: true
You should see the deprecation logged to the console
Deprecation should also be logged if <my-plugin>.enabled: false
To test with the upgrade assistant, pull down the 7.x backport PR and repeat steps 1-3: [7.x] Log deprecation warnings for plugins which won't be disable-able in 8.0 (#112602) #112728 You should be able to see a critical deprecation notice under Stack Management > Upgrade Assistant

I also tried to find any documentation that referred to a plugin with an .enabled property. If the docs for your app have been changed, please give them a look. (Preview: https://kibana_112602.docs-preview.app.elstc.co/diff)

This change will be backported to 7.16. In a follow-up PR, we will implement the actual 8.0 breaking change by removing the deprecated config.

Notes for Core Team

The approach here does the following:

Adds a deprecation from the config service for any plugin with an .enabled config that's implicitly added (i.e., plugins which currently have no config schema)
Adds a config schema to a few vis_type_* plugins which currently don't have one but will need to keep an .enabled config in 8.0
Manually add a deprecation warning to plugins which already have an .enabled config that's explicitly added in their config schema
- To make this easier I've added a new deprecate and deprecateFromRoot to the ConfigDeprecationFactory
Updates docs for any references to deprecated .enabled properties.

I went with this approach because:

It's simple and explicit
We will already need to manually change every existing config schema with an .enabled property in 8.0, so this is just a precursor to that PR
- Sure, this means 2 PRs where we ping a bunch of codeowners instead of just one, but it also gives these teams a heads up
It doesn't require any special treatment for 3rd party plugins:
- 3rd party plugins which already have a config schema & an .enabled property are unaffected
- 1st party and 3rd party plugins which currently have no config schema are treated in the same way (deprecation is logged)
- If any 1st or 3rd party plugin wants to support an .enabled property moving forward, they just add it to their config schema

However, I'm still open to feedback on alternatives.

Plugin API Changes

The ability for most plugins to be disabled using the {plugin_name}.enabled config option has been deprecated. In 8.0, most Kibana plugins can no longer be disabled using this option.

Plugin developers can still opt-in to this feature by explicitly adding an enabled property to their config schema. However, we recommend against this when possible; it affects whether or not a plugin's code is loaded by Kibana's core, thus introducing complexity and creating a new set of configuration scenarios that must be tested.

If you would like to make some aspects of your plugin disable-able, for example the ability to remove it from Kibana's UI entirely, we recommend instead creating "nested" configuration options, e.g. {plugin_name}.ui.enabled instead of {plugin_name}.enabled, and then reading from the configuration at runtime to conditionally render your application. This gives you similar functionality without preventing your plugin code from loading altogether.

Deprecation docs

The ability for most plugins to be disabled using the {plugin_name}.enabled config option has been deprecated. In 8.0, most Kibana plugins can no longer be disabled using this option. If you are currently using one of these options in your Kibana config, please remove it before upgrading to 8.0.

…rward.

elasticmachine · 2021-09-20T18:56:23Z

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

lukeelmers · 2021-09-21T20:02:25Z

For reviewers who would like to test how this looks with the Upgrade Assistant, I have a PR opened that will manually backport this to 7.x #112728

(We still need to merge this PR into master as it adds some new APIs to Core's ConfigDeprecationFactory which we will be preserving moving forward)

lukeelmers · 2021-09-21T20:02:35Z

@elasticmachine merge upstream

nreese

maps changes LGTM
code review

mgiota

Infra changes LGTM! I tested locally and I can verify:

I got the deprecation warning in my Kibana server
the issue appeared under Kibana deprecation issues in Upgrade assistant

One thing I should note here is that I got following error when I clicked on the Enable deprecation logging and indexing toggle of step3 in the Upgrade Assistant only the first time. Clicking on the toggle again didn't produce any error.

@Kerry350 Can you verify that we don't make use of this flag anywhere in the infra plugin? I searched for it and didn't find anything.

Kerry350 · 2021-09-22T10:58:15Z

@mgiota

@Kerry350 Can you verify that we don't make use of this flag anywhere in the infra plugin? I searched for it and didn't find anything.

I don't recall us ever using it. And I can't find anything either. LGTM.

patrykkopycinski

Asset management LGTM

lukeelmers · 2021-09-22T14:44:34Z

@mgiota @Kerry350

Can you verify that we don't make use of this flag anywhere in the infra plugin? I searched for it and didn't find anything.

I don't recall us ever using it. And I can't find anything either. LGTM.

Just to provide some context here, the enabled configuration option isn't something you'd ever use inside of your plugin, because core uses it during the plugin discovery process to determine whether to load your plugin at all. If enabled: false is set, your plugin code will never run.

So the impact here would be on any users are relying on disabling the infra plugin via the config, as that is the functionality which will be going away in 8.0.

One thing I should note here is that I got following error when I clicked on the Enable deprecation logging and indexing toggle of step3 in the Upgrade Assistant only the first time. Clicking on the toggle again didn't produce any error.

I discussed this with @cjcenizal and he said it is a known issue with the Upgrade Assistant that is currently being worked on. Thanks for calling it out!

lukeelmers · 2021-09-22T14:50:47Z

@elasticmachine merge upstream

ogupte

APM changes LGTM

kibanamachine · 2021-09-22T19:58:54Z

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`core`	1014	1020	+6

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`upgradeAssistant`	18.8KB	18.8KB	-29.0B

Unknown metric groups

API count

id	before	after	diff
`core`	2284	2292	+8

History

💚 Build #154998 succeeded 7b81e46
💚 Build #154747 succeeded 13dc186
💚 Build #154748 succeeded 897d0df
💚 Build #154364 succeeded 5561a88

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

cc @lukeelmers

…e in 8.0 (#112602) (#112728)

…-migrate-away-from-injected-css-js * 'master' of github.com:elastic/kibana: (189 commits) fix permissions for cloud test (elastic#112568) Updates the VEGA docs for v8.0 (elastic#112781) Updates the TSVB docs for v8 (elastic#112778) [Expressions] Fix setup and start contracts (elastic#110841) [DOCS] Update remote cluster and security links (elastic#112874) test/functional/apps/management/_test_huge_fields.js (elastic#112878) Fix the other one... (elastic#112873) [data.search.aggs] Use fields instead of _source in top_hits agg (elastic#109531) [Search sessions] Don't show incomplete warning if search requests aren't in session (elastic#112364) [data.search] Do not send ignore_throttled when search:includeFrozen is disabled (elastic#112755) [Monitoring] Add KQL filter bar to alerts (elastic#111663) Log deprecation warnings for plugins which won't be disable-able in 8.0 (elastic#112602) [CI] Balance CI Groups (elastic#112836) Add ILM URLs to documentation link service (elastic#112748) Bump chromedriver to 93 (elastic#112847) [Maps] move joins from LayerDescriptor to VectorLayerDescriptor (elastic#112427) Add a handler for a possible promise rejection (elastic#112840) Removes space, fix build (elastic#112856) [Maps] fix unhandled promise rejections in jest tests (elastic#112712) Copy pass 3 (elastic#112815) ... # Conflicts: # src/plugins/dashboard/public/application/dashboard_app.tsx # src/plugins/dashboard/public/application/embeddable/viewport/dashboard_viewport.tsx

lukeelmers added 7 commits September 20, 2021 12:36

Log deprecations for implicitly disable-able plugins.

c705030

Add explicit 'enabled' option to plugins which will need it moving fo…

6a4b086

…rward.

Add manual deprecation as an example.

e1340c1

Add 'deprecate' and 'deprecateFromRoot' to ConfigDeprecationFactory.

14fc7ce

Deprecate 'enabled' config for plugins with expicit config schemas.

83b9f80

Add deprecation notices to the docs.

5df2e2a

Bump stdout maxListeners in legacy logging.

5561a88

lukeelmers self-assigned this Sep 20, 2021

lukeelmers requested a review from a team as a code owner September 20, 2021 18:56

lukeelmers requested a review from a team September 20, 2021 18:56

lukeelmers requested review from a team as code owners September 20, 2021 18:56

lukeelmers requested a review from a team September 20, 2021 18:56

lukeelmers requested review from a team as code owners September 20, 2021 18:56

Merge branch 'master' into feat/disableable-plugins

897d0df

nreese approved these changes Sep 21, 2021

View reviewed changes

lukeelmers added the backport:skip This commit does not require backporting label Sep 21, 2021

mgiota approved these changes Sep 21, 2021

View reviewed changes

kqualters-elastic approved these changes Sep 22, 2021

View reviewed changes

joshdover mentioned this pull request Sep 22, 2021

[Fleet] Add custom integrations API #112481

Merged

2 tasks

patrykkopycinski approved these changes Sep 22, 2021

View reviewed changes

kibanamachine and others added 2 commits September 22, 2021 10:50

Merge branch 'master' into feat/disableable-plugins

7b81e46

Fix typo in deprecation factory.

e83b6c9

lukeelmers requested a review from a team September 22, 2021 17:17

jonathan-buttner approved these changes Sep 22, 2021

View reviewed changes

ogupte approved these changes Sep 22, 2021

View reviewed changes

Fix missed typo

eba0043

lukeelmers changed the title ~~Log deprecation warnings for plugins which will no longer be disable-able in 8.0~~ Log deprecation warnings for plugins which won't be disable-able in 8.0 Sep 22, 2021

lukeelmers enabled auto-merge (squash) September 22, 2021 17:53

lukeelmers merged commit 878b1ee into elastic:master Sep 22, 2021

lukeelmers deleted the feat/disableable-plugins branch September 22, 2021 20:05

lukeelmers added a commit that referenced this pull request Sep 22, 2021

[7.x] Log deprecation warnings for plugins which won't be disable-abl…

ef8518d

…e in 8.0 (#112602) (#112728)

This was referenced Oct 5, 2021

[Breaking] Remove deprecated enabled settings from plugins. #113495

Merged

[meta] Improve deprecation warnings for plugins without an enabled config in 8.0. #114188

Closed

jportner mentioned this pull request Oct 6, 2021

[Meta] Platform Security team - 8.0 upgrade work #111160

Closed

yuliacech mentioned this pull request Oct 11, 2021

ILM is not disabled when Index Management is disabled #88027

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log deprecation warnings for plugins which won't be disable-able in 8.0 #112602

Log deprecation warnings for plugins which won't be disable-able in 8.0 #112602

lukeelmers commented Sep 20, 2021 •

edited

Loading

elasticmachine commented Sep 20, 2021

lukeelmers commented Sep 21, 2021

lukeelmers commented Sep 21, 2021

nreese left a comment

mgiota left a comment •

edited

Loading

Kerry350 commented Sep 22, 2021

patrykkopycinski left a comment

lukeelmers commented Sep 22, 2021

lukeelmers commented Sep 22, 2021

ogupte left a comment

kibanamachine commented Sep 22, 2021

API count

Log deprecation warnings for plugins which won't be disable-able in 8.0 #112602

Log deprecation warnings for plugins which won't be disable-able in 8.0 #112602

Conversation

lukeelmers commented Sep 20, 2021 • edited Loading

Notes if you are pinged for a CODEOWNERS review

Notes for Core Team

Plugin API Changes

Deprecation docs

elasticmachine commented Sep 20, 2021

lukeelmers commented Sep 21, 2021

lukeelmers commented Sep 21, 2021

nreese left a comment

Choose a reason for hiding this comment

mgiota left a comment • edited Loading

Choose a reason for hiding this comment

Kerry350 commented Sep 22, 2021

patrykkopycinski left a comment

Choose a reason for hiding this comment

lukeelmers commented Sep 22, 2021

lukeelmers commented Sep 22, 2021

ogupte left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 22, 2021

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Page load bundle

API count

History

lukeelmers commented Sep 20, 2021 •

edited

Loading

mgiota left a comment •

edited

Loading