add synchronous config access API

[pgayvallet]

Summary

Fix #82723

PR adds a new PluginInitializerContext.config.get API to retrieve the current value of the plugin's configuration synchronously.

• Add ConfigService.atPathSync and PluginInitializerContext.config.get
• Remove the unsed PluginInitializerContext.config.createIfExists API
• Cleanup the ConfigService, mostly the markAsHandled logic (see review comment)
• Add TS doc on PluginInitializerContext

Example

 // plugins/my-plugin/server/plugin.ts

 export class MyPlugin implements Plugin {
   constructor(private readonly initContext: PluginInitializerContext) {}
   setup(core) {
     const config = this.initContext.config.get<MyPluginConfigType>();
     // do something with the config
   }
 }

Checklist

• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios

[pgayvallet]

Method was async for now reason. Also adapted the consumers

[pgayvallet]

We were using an array instead of a set to store the used paths, meaning that we were eventually having duplicates. (Impact was minimal as we are using a shareReplay in the plugin context's config.create observable, but still)

[pgayvallet]

We were flagging configurations as handled every time we called atPath. That probably made sense in the initial implementation, but since we now validate the whole config at startups, there are now unnecessary calls.

The only calls to markAsHandled remaining are the ones done during config validation, and when invoking isEnabledAtPath to mark a disabled plugin's path as handled.

[mshustov]

Can we move validateAtPath to perform validation only once on every config$ emit? Then we get rid of validation overhead on every config read operation. WDYT? Can be done in a separate PR.a

[pgayvallet]

Validating the config within the whole config obs

this.config$ = combineLatest([this.rawConfigProvider.getConfig$(), this.deprecations]).pipe(...

May be tricky because we would need to already prepare each child observable here, one per schema (we don't want this.config$ to error.).

In a similar way, ideally, atPathSync should only validate the config once and memoize the error/validation result until next emission.

Also, as you said on slack, the timing between emissions and schema registration needs to be taken into account here. When registering a schema, we need to validate the associated part of the config.

PluginInitializerContext.config is not an API that should be used a lot, even during plugin initialization (theoretically the config should only be read once, more or less, per plugin), so this should be minor.

I agree that this is still a performance improvement though, I will create an issue.

[pgayvallet]

During testing, directly calling atPathSync after instantiating the service was causing errors because this.config$ has not emitted yet. Even if that would never occur in real scenarios, as we are awaiting for the config to validate before doing anything with it, I added an explicit rule requiring validate to have been called before being able to use atPathSync

[elasticmachine]

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

[Bamieh]

Code LGTM!

Syncing my discussion with pierre about this feature:

The observable based implementation was meant to allow users to reconfigure Kibana at runtime.

hotreload is not active at the moment. We are not emitting a new config to plugins, and manually calling loggingService.reload instead.

Implementing hotreload will require a lot of effort and has several complexities like disabling a previously enabled plugin.

Sync lifecycle has its own benefits and cons but that was the compromise the team decided to take at some point.

[mshustov]

How are we going to communicate this change to the Solution teams? Should we create a meta issue to track migration progress for sync lifecycle methods?

[mshustov]

Such an irony, considering that we are removing the only config value elasticsearch.logQueries supporting reactivity.

[rudolf]

YAGNI 😉

[mshustov]

Can we move validateAtPath to perform validation only once on every config$ emit? Then we get rid of validation overhead on every config read operation. WDYT? Can be done in a separate PR.a

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: b833984

Metrics [docs]

✅ unchanged

History

• 💔 Build #101688 failed cafc7a6
• 💚 Build #101030 succeeded 1a5a47e
• 💔 Build #101020 failed 2c7579e
• 💔 Build #101009 failed 5a6a761
• 💔 Build #101006 failed 8fbb27b

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