Kibana deprecation API shouldn't default level to critical

[cjcenizal]

Per https://github.com/elastic/kibana/blob/master/src/core/server/deprecations/deprecations_service.ts#L190, deprecations default to level: critical if one isn't provided.

Default values are unhelpful in this case, because we want the consumer to be very thoughtful and deliberate when choosing whether a deprecation should be critical (thereby blocking the upgrade) or a warning (thereby optional). A default value makes it easy for developers to overlook this important decision. In contrast, forcing developers to be explicit about the level will nudge them to consider which level is most appropriate for the deprecation.

[elasticmachine]

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

[lukeelmers]

@cjcenizal So basically you are proposing that we make level required instead of optional?

These are the current DeprecatedConfigDetails

kibana/packages/kbn-config/src/deprecation/types.ts

Lines 22 to 45 in 58bab91

	export interface DeprecatedConfigDetails {
	/** The title to be displayed for the deprecation. */
	title?: string;
	/** The message to be displayed for the deprecation. */
	message: string;
	/**
	* levels:
	* - warning: will not break deployment upon upgrade
	* - critical: needs to be addressed before upgrade.
	*/
	level?: 'warning' | 'critical';
	/** (optional) set false to prevent the config service from logging the deprecation message. */
	silent?: boolean;
	/** (optional) link to the documentation for more details on the deprecation. */
	documentationUrl?: string;
	/** corrective action needed to fix this deprecation. */
	correctiveActions: {
	/**
	* Specify a list of manual steps our users need to follow
	* to fix the deprecation before upgrade.
	*/
	manualSteps: string[];
	};
	}

[cjcenizal]

So basically you are proposing that we make level required instead of optional?

Correct.

[pgayvallet]

So basically you are proposing that we make level required instead of optional?

It does make sense imho. Such significant fields should always be explicitly specified instead of allowing to fallback to an opinionated default.

[lukeelmers]

Yeah agree, this makes sense to me too, just wanted to make sure I wasn't missing something.

I'll add it to our list so we can discuss if it's possible to squeeze in. Wrangling all of the codeowners will be the tedious part here; changing the service is straightforward.

[cjcenizal]

@pgayvallet @lukeelmers To me, the primary goal is to ensure that every deprecation that's been registered as critical really does need to be critical. Which implies that this process should include some kind of review or audit.

Theoretically, we could say that a deprecation that's been registered with the wrong level is a bug, so could be fixed after FF.

Can we approach this on two tracks?

• Ping every team for a level revision review. This can extend past FF and is the most critical piece of work, IMO.
• Make level required instead of optional. This is has low criticality, since we're so close to FF and I don't expect many teams will be registering new deprecations.

Critical vs warning

Devs will also need to know how to decide whether a deprecation is critical or warning.

• Critical deprecations are ones that must be addressed before the upgrade, otherwise the deployment will be unstable, degraded, fail to start, or otherwise exhibit undesirable and unintended behavior.
• Warning deprecations are everything else. They're optional or they may not apply to the user. The user has discretion to decide whether or not to act on them. If the user ignores them and upgrades the deployment, the deployment will function normally.

[lukeelmers]

Thanks @cjcenizal -- was just going to ping you today to suggest something similar 🙂

Right now it doesn't look like we'll be able to fit in the work to make the field required instead of optional in time for feature freeze, mainly due to the need to coordinate with all teams.

So I'm +1 on your suggestion to start by pinging every team to do an audit and decide for themselves whether a critical deprecation is warranted.

We will then take changing the default as a follow-up task.

@pgayvallet As you are working through the other changes to the deprecation API, would you be willing to create a separate issue that we can use to ping all of the affected teams (since I think you said you already had an inventory of them)? Then we can keep the scope of this issue focused on changing the default, and slot it in for sometime in the near-ish future.

[pgayvallet]

you be willing to create a separate issue that we can use to ping all of the affected teams (since I think you said you already had an inventory of them)

Sure, will do. Just a few comments and a question first.

Note that I only have a list of non-factory config deprecation registered via PluginConfigDescriptor (and from master only) in #114751. I will have the equivalent list of 7.x once I backport and adapt the PR for 7.x-only deprecations.

Here, we'll also need the inventory of features deprecations registered via the deprecations core API. But nothing that some elite grep skills can't address.

Hardest part is the current divergence in term of deprecations between master and 7.x. Our priority is to set the correct level in 7.x, but we also want to do it on master (even if a lot of deprecations are no longer present in that branch). So we probably need to do two lists.

We have a remaining question here: Should we also enforce every factory-based (e.g rename, unused) config deprecation to also define a level?

e.g, atm, an unused deprecation can be defined as:

export const config: PluginConfigDescriptor<ConfigType> = {
  schema,
  deprecations: ({ unused }) => [
    unused('ssl'),
  ],
};

it's currently possible to define the level, even if that's optional, via:

export const config: PluginConfigDescriptor<ConfigType> = {
  schema,
  deprecations: ({ unused }) => [
    unused('ssl', { level: 'warning' }),
  ],
};

do we want to also make level mandatory for this kind of deprecations, and force this level option to be specified for all factory-based deprecations, or are we good keeping this optional with a default of critical? If we do want to explicitly specify the level, I will need to also list all usages of factory-based config deprecations.

[cjcenizal]

@pgayvallet Thanks for outlining your approach. Great question. Given that our goal is to double-check all critical deprecations and re-level any that should be warnings, I think we do want to make level mandatory for all factory-based deprecations.

In terms of DX, I could imagine someone making the case that developers will benefit from factory-specific defaults, and that they could intuit the default level value should be critical given that these factories imply hard breaking changes. But TBH I haven’t used them enough to be confident to make that case myself, so I fall back on the weaker argument of striving for consistency: if level is mandatory in one place it should be mandatory everywhere.

[pgayvallet]

but TBH I haven’t used them enough to be confident to make that case myself, so I fall back on the weaker argument of striving for consistency: if level is mandatory in one place it should be mandatory everywhere.

Yea, it all depends on the target version (the version we're planning to effectively remove the config setting)

e.g

rename('foo', 'bar')

in 7.x would be critical if the deprecation get removed in 8.0, but only warning if we're planning to remove it in a later version. So I don't think we can (atm) have 'smart' default for those factory deprecations.

[lukeelmers]

Yea, it all depends on the target version (the version we're planning to effectively remove the config setting)

Yep 💯 -- While I much prefer the DX of not having to specify a level, it feels like we won't really have much of a choice but to make it required for the factory-based deprecations as well.

Otherwise there will always be a scenario where the level is either too "low" or too "high", depending on the branch.

[pgayvallet]

Created #115344 to track adding level on existing deprecations