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

add synchronous config access API #88981

Merged
merged 9 commits into from
Jan 28, 2021
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

## PluginInitializerContext.config property

Accessors for the plugin's configuration

<b>Signature:</b>

```typescript
Expand All @@ -12,6 +14,6 @@ config: {
globalConfig$: Observable<SharedGlobalConfig>;
};
create: <T = ConfigSchema>() => Observable<T>;
createIfExists: <T = ConfigSchema>() => Observable<T | undefined>;
get: <T = ConfigSchema>() => T;
};
```
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,29 @@

## PluginInitializerContext.logger property

instance already bound to the plugin's logging context

<b>Signature:</b>

```typescript
logger: LoggerFactory;
```

## Example


```typescript
// plugins/my-plugin/server/plugin.ts
// "id: myPlugin" in `plugins/my-plugin/kibana.yaml`

export class MyPlugin implements Plugin {
constructor(private readonly initContext: PluginInitializerContext) {
this.logger = initContext.logger.get();
// `logger` context: `plugins.myPlugin`
this.mySubLogger = initContext.logger.get('sub'); // or this.logger.get('sub');
// `mySubLogger` context: `plugins.myPlugin.sub`
}
}

```

Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ export interface PluginInitializerContext<ConfigSchema = unknown>

| Property | Type | Description |
| --- | --- | --- |
| [config](./kibana-plugin-core-server.plugininitializercontext.config.md) | <code>{</code><br/><code> legacy: {</code><br/><code> globalConfig$: Observable&lt;SharedGlobalConfig&gt;;</code><br/><code> };</code><br/><code> create: &lt;T = ConfigSchema&gt;() =&gt; Observable&lt;T&gt;;</code><br/><code> createIfExists: &lt;T = ConfigSchema&gt;() =&gt; Observable&lt;T &#124; undefined&gt;;</code><br/><code> }</code> | |
| [config](./kibana-plugin-core-server.plugininitializercontext.config.md) | <code>{</code><br/><code> legacy: {</code><br/><code> globalConfig$: Observable&lt;SharedGlobalConfig&gt;;</code><br/><code> };</code><br/><code> create: &lt;T = ConfigSchema&gt;() =&gt; Observable&lt;T&gt;;</code><br/><code> get: &lt;T = ConfigSchema&gt;() =&gt; T;</code><br/><code> }</code> | Accessors for the plugin's configuration |
| [env](./kibana-plugin-core-server.plugininitializercontext.env.md) | <code>{</code><br/><code> mode: EnvironmentMode;</code><br/><code> packageInfo: Readonly&lt;PackageInfo&gt;;</code><br/><code> instanceUuid: string;</code><br/><code> }</code> | |
| [logger](./kibana-plugin-core-server.plugininitializercontext.logger.md) | <code>LoggerFactory</code> | |
| [logger](./kibana-plugin-core-server.plugininitializercontext.logger.md) | <code>LoggerFactory</code> | instance already bound to the plugin's logging context |
| [opaqueId](./kibana-plugin-core-server.plugininitializercontext.opaqueid.md) | <code>PluginOpaqueId</code> | |

3 changes: 2 additions & 1 deletion packages/kbn-config/src/config_service.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ const createConfigServiceMock = ({
}: { atPath?: Record<string, any>; getConfig$?: Record<string, any> } = {}) => {
const mocked: jest.Mocked<IConfigService> = {
atPath: jest.fn(),
atPathSync: jest.fn(),
getConfig$: jest.fn(),
optionalAtPath: jest.fn(),
getUsedPaths: jest.fn(),
getUnusedPaths: jest.fn(),
isEnabledAtPath: jest.fn(),
Expand All @@ -27,6 +27,7 @@ const createConfigServiceMock = ({
validate: jest.fn(),
};
mocked.atPath.mockReturnValue(new BehaviorSubject(atPath));
mocked.atPathSync.mockReturnValue(atPath);
mocked.getConfig$.mockReturnValue(new BehaviorSubject(new ObjectToConfigAdapter(getConfig$)));
mocked.getUsedPaths.mockResolvedValue([]);
mocked.getUnusedPaths.mockResolvedValue([]);
Expand Down
112 changes: 66 additions & 46 deletions packages/kbn-config/src/config_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,27 +105,6 @@ test('re-validate config when updated', async () => {
`);
});

test("returns undefined if fetching optional config at a path that doesn't exist", async () => {
const rawConfig = getRawConfigProvider({});
const configService = new ConfigService(rawConfig, defaultEnv, logger);

const value$ = configService.optionalAtPath('unique-name');
const value = await value$.pipe(first()).toPromise();

expect(value).toBeUndefined();
});

test('returns observable config at optional path if it exists', async () => {
const rawConfig = getRawConfigProvider({ value: 'bar' });
const configService = new ConfigService(rawConfig, defaultEnv, logger);
await configService.setSchema('value', schema.string());

const value$ = configService.optionalAtPath('value');
const value: any = await value$.pipe(first()).toPromise();

expect(value).toBe('bar');
});

test("does not push new configs when reloading if config at path hasn't changed", async () => {
const rawConfig$ = new BehaviorSubject<Record<string, any>>({ key: 'value' });
const rawConfigProvider = rawConfigServiceMock.create({ rawConfig$ });
Expand Down Expand Up @@ -209,34 +188,38 @@ test('flags schema paths as handled when registering a schema', async () => {

test('tracks unhandled paths', async () => {
const initialConfig = {
bar: {
deep1: {
key: '123',
},
deep2: {
key: '321',
},
service: {
string: 'str',
number: 42,
},
foo: 'value',
quux: {
deep1: {
key: 'hello',
},
deep2: {
key: 'world',
},
plugin: {
foo: 'bar',
},
unknown: {
hello: 'dolly',
number: 9000,
},
};

const rawConfigProvider = rawConfigServiceMock.create({ rawConfig: initialConfig });
const configService = new ConfigService(rawConfigProvider, defaultEnv, logger);

configService.atPath('foo');
configService.atPath(['bar', 'deep2']);
await configService.setSchema(
'service',
schema.object({
string: schema.string(),
number: schema.number(),
})
);
await configService.setSchema(
'plugin',
schema.object({
foo: schema.string(),
})
);

const unused = await configService.getUnusedPaths();

expect(unused).toEqual(['bar.deep1.key', 'quux.deep1.key', 'quux.deep2.key']);
expect(unused).toEqual(['unknown.hello', 'unknown.number']);
});

test('correctly passes context', async () => {
Expand Down Expand Up @@ -339,22 +322,18 @@ test('does not throw if schema does not define "enabled" schema', async () => {

const rawConfigProvider = rawConfigServiceMock.create({ rawConfig: initialConfig });
const configService = new ConfigService(rawConfigProvider, defaultEnv, logger);
await expect(
expect(
configService.setSchema(
'pid',
schema.object({
file: schema.string(),
})
)
).resolves.toBeUndefined();
).toBeUndefined();

const value$ = configService.atPath('pid');
const value: any = await value$.pipe(first()).toPromise();
expect(value.enabled).toBe(undefined);

const valueOptional$ = configService.optionalAtPath('pid');
const valueOptional: any = await valueOptional$.pipe(first()).toPromise();
expect(valueOptional.enabled).toBe(undefined);
});

test('treats config as enabled if config path is not present in config', async () => {
Expand Down Expand Up @@ -457,3 +436,44 @@ test('logs deprecation warning during validation', async () => {
]
`);
});

describe('atPathSync', () => {
test('returns the value at path', async () => {
const rawConfig = getRawConfigProvider({ key: 'foo' });
const configService = new ConfigService(rawConfig, defaultEnv, logger);
const stringSchema = schema.string();
await configService.setSchema('key', stringSchema);

await configService.validate();

const value = configService.atPathSync('key');
expect(value).toBe('foo');
});

test('throws if called before `validate`', async () => {
const rawConfig = getRawConfigProvider({ key: 'foo' });
const configService = new ConfigService(rawConfig, defaultEnv, logger);
const stringSchema = schema.string();
await configService.setSchema('key', stringSchema);

expect(() => configService.atPathSync('key')).toThrowErrorMatchingInlineSnapshot(
`"\`atPathSync\` called before config was validated"`
);
});

test('returns the last config value', async () => {
const rawConfig$ = new BehaviorSubject<Record<string, any>>({ key: 'value' });
const rawConfigProvider = rawConfigServiceMock.create({ rawConfig$ });

const configService = new ConfigService(rawConfigProvider, defaultEnv, logger);
await configService.setSchema('key', schema.string());

await configService.validate();

expect(configService.atPathSync('key')).toEqual('value');

rawConfig$.next({ key: 'new-value' });

expect(configService.atPathSync('key')).toEqual('new-value');
});
});
63 changes: 29 additions & 34 deletions packages/kbn-config/src/config_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import type { PublicMethodsOf } from '@kbn/utility-types';
import { Type } from '@kbn/config-schema';
import { isEqual } from 'lodash';
import { BehaviorSubject, combineLatest, Observable } from 'rxjs';
import { distinctUntilChanged, first, map, shareReplay, take } from 'rxjs/operators';
import { distinctUntilChanged, first, map, shareReplay, take, tap } from 'rxjs/operators';
import { Logger, LoggerFactory } from '@kbn/logging';

import { Config, ConfigPath, Env } from '.';
Expand All @@ -32,13 +32,15 @@ export class ConfigService {
private readonly log: Logger;
private readonly deprecationLog: Logger;

private validated = false;
private readonly config$: Observable<Config>;
private lastConfig?: Config;

/**
* Whenever a config if read at a path, we mark that path as 'handled'. We can
* then list all unhandled config paths when the startup process is completed.
*/
private readonly handledPaths: ConfigPath[] = [];
private readonly handledPaths: Set<ConfigPath> = new Set();
Comment on lines -41 to +43
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

private readonly schemas = new Map<string, Type<unknown>>();
private readonly deprecations = new BehaviorSubject<ConfigDeprecationWithContext[]>([]);

Expand All @@ -55,14 +57,17 @@ export class ConfigService {
const migrated = applyDeprecations(rawConfig, deprecations);
return new LegacyObjectToConfigAdapter(migrated);
}),
tap((config) => {
this.lastConfig = config;
}),
shareReplay(1)
);
}

/**
* Set config schema for a path and performs its validation
*/
public async setSchema(path: ConfigPath, schema: Type<unknown>) {
public setSchema(path: ConfigPath, schema: Type<unknown>) {
Comment on lines -65 to +70
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Method was async for now reason. Also adapted the consumers

const namespace = pathToString(path);
if (this.schemas.has(namespace)) {
throw new Error(`Validation schema for [${path}] was already registered.`);
Expand Down Expand Up @@ -94,43 +99,43 @@ export class ConfigService {
public async validate() {
const namespaces = [...this.schemas.keys()];
for (let i = 0; i < namespaces.length; i++) {
await this.validateConfigAtPath(namespaces[i]).pipe(first()).toPromise();
await this.getValidatedConfigAtPath$(namespaces[i]).pipe(first()).toPromise();
}

await this.logDeprecation();
this.validated = true;
}

/**
* Returns the full config object observable. This is not intended for
* "normal use", but for features that _need_ access to the full object.
* "normal use", but for internal features that _need_ access to the full object.
*/
public getConfig$() {
return this.config$;
}

/**
* Reads the subset of the config at the specified `path` and validates it
* against the static `schema` on the given `ConfigClass`.
* against its registered schema.
*
* @param path - The path to the desired subset of the config.
*/
public atPath<TSchema>(path: ConfigPath) {
return this.validateConfigAtPath(path) as Observable<TSchema>;
return this.getValidatedConfigAtPath$(path) as Observable<TSchema>;
}

/**
* Same as `atPath`, but returns `undefined` if there is no config at the
* specified path.
* Similar to {@link atPath}, but return the last emitted value synchronously instead of an
* observable.
*
* {@link ConfigService.atPath}
* @param path - The path to the desired subset of the config.
*/
public optionalAtPath<TSchema>(path: ConfigPath) {
return this.getDistinctConfig(path).pipe(
map((config) => {
if (config === undefined) return undefined;
return this.validateAtPath(path, config) as TSchema;
})
);
public atPathSync<TSchema>(path: ConfigPath) {
if (!this.validated) {
throw new Error('`atPathSync` called before config was validated');
}
const configAtPath = this.lastConfig!.get(path);
return this.validateAtPath(path, configAtPath) as TSchema;
Comment on lines +133 to +138
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

public async isEnabledAtPath(path: ConfigPath) {
Expand All @@ -144,10 +149,7 @@ export class ConfigService {
const config = await this.config$.pipe(first()).toPromise();

// if plugin hasn't got a config schema, we try to read "enabled" directly
const isEnabled =
validatedConfig && validatedConfig.enabled !== undefined
? validatedConfig.enabled
: config.get(enabledPath);
const isEnabled = validatedConfig?.enabled ?? config.get(enabledPath);

// not declared. consider that plugin is enabled by default
if (isEnabled === undefined) {
Expand All @@ -170,15 +172,13 @@ export class ConfigService {

public async getUnusedPaths() {
const config = await this.config$.pipe(first()).toPromise();
const handledPaths = this.handledPaths.map(pathToString);

const handledPaths = [...this.handledPaths.values()].map(pathToString);
return config.getFlattenedPaths().filter((path) => !isPathHandled(path, handledPaths));
}

public async getUsedPaths() {
const config = await this.config$.pipe(first()).toPromise();
const handledPaths = this.handledPaths.map(pathToString);

const handledPaths = [...this.handledPaths.values()].map(pathToString);
return config.getFlattenedPaths().filter((path) => isPathHandled(path, handledPaths));
}

Expand Down Expand Up @@ -210,22 +210,17 @@ export class ConfigService {
);
}

private validateConfigAtPath(path: ConfigPath) {
return this.getDistinctConfig(path).pipe(map((config) => this.validateAtPath(path, config)));
}

private getDistinctConfig(path: ConfigPath) {
this.markAsHandled(path);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


private getValidatedConfigAtPath$(path: ConfigPath) {
return this.config$.pipe(
map((config) => config.get(path)),
distinctUntilChanged(isEqual)
distinctUntilChanged(isEqual),
map((config) => this.validateAtPath(path, config))
);
}

private markAsHandled(path: ConfigPath) {
this.log.debug(`Marking config path as handled: ${path}`);
this.handledPaths.push(path);
this.handledPaths.add(path);
}
}

Expand Down
Loading