diff --git a/src/plugins/usage_collection/README.md b/src/plugins/usage_collection/README.md index 9520dfc03cfa47..a828096f86042e 100644 --- a/src/plugins/usage_collection/README.md +++ b/src/plugins/usage_collection/README.md @@ -59,7 +59,7 @@ All you need to provide is a `type` for organizing your fields, `schema` field t // create usage collector const myCollector = usageCollection.makeUsageCollector({ - type: MY_USAGE_TYPE, + type: 'MY_USAGE_TYPE', schema: { my_objects: { total: 'long', @@ -84,7 +84,11 @@ All you need to provide is a `type` for organizing your fields, `schema` field t } ``` -Some background: The `callCluster` that gets passed to the `fetch` method is created in a way that's a bit tricky, to support multiple contexts the `fetch` method could be called. Your `fetch` method could get called as a result of an HTTP API request: in this case, the `callCluster` function wraps `callWithRequest`, and the request headers are expected to have read privilege on the entire `.kibana` index. The use case for this is stats pulled from a Kibana Metricbeat module, where the Beat calls Kibana's stats API in Kibana to invoke collection. +Some background: + +- `MY_USAGE_TYPE` can be any string. It usually matches the plugin name. As a safety mechanism, we double check there are no duplicates at the moment of registering the collector. +- The `fetch` method needs to support multiple contexts in which it is called. For example, when stats are pulled from a Kibana Metricbeat module, the Beat calls Kibana's stats API to invoke usage collection. +In this case, the `fetch` method is called as a result of an HTTP API request and `callCluster` wraps `callWithRequest`, where the request headers are expected to have read privilege on the entire `.kibana' index. Note: there will be many cases where you won't need to use the `callCluster` function that gets passed in to your `fetch` method at all. Your feature might have an accumulating value in server memory, or read something from the OS, or use other clients like a custom SavedObjects client. In that case it's up to the plugin to initialize those clients like the example below: diff --git a/src/plugins/usage_collection/server/collector/collector_set.test.ts b/src/plugins/usage_collection/server/collector/collector_set.test.ts index 50919ecb3d83f8..545642c5dcfa3b 100644 --- a/src/plugins/usage_collection/server/collector/collector_set.test.ts +++ b/src/plugins/usage_collection/server/collector/collector_set.test.ts @@ -41,7 +41,7 @@ describe('CollectorSet', () => { loggerSpies.warn.mockRestore(); }); - const mockCallCluster = () => Promise.resolve({ passTest: 1000 }); + const mockCallCluster = jest.fn().mockResolvedValue({ passTest: 1000 }); it('should throw an error if non-Collector type of object is registered', () => { const collectors = new CollectorSet({ logger }); @@ -58,6 +58,23 @@ describe('CollectorSet', () => { ); }); + it('should throw when 2 collectors with the same type are registered', () => { + const collectorSet = new CollectorSet({ logger }); + collectorSet.registerCollector( + new Collector(logger, { type: 'test_duplicated', fetch: () => 1, isReady: () => true }) + ); + expect(() => + collectorSet.registerCollector( + // Even for Collector vs. UsageCollector + new UsageCollector(logger, { + type: 'test_duplicated', + fetch: () => 2, + isReady: () => false, + }) + ) + ).toThrowError(`Usage collector's type "test_duplicated" is duplicated.`); + }); + it('should log debug status of fetching from the collector', async () => { const collectors = new CollectorSet({ logger }); collectors.registerCollector( @@ -68,7 +85,7 @@ describe('CollectorSet', () => { }) ); - const result = await collectors.bulkFetch(mockCallCluster as any); + const result = await collectors.bulkFetch(mockCallCluster); expect(loggerSpies.debug).toHaveBeenCalledTimes(1); expect(loggerSpies.debug).toHaveBeenCalledWith( 'Fetching data from MY_TEST_COLLECTOR collector' @@ -93,7 +110,7 @@ describe('CollectorSet', () => { let result; try { - result = await collectors.bulkFetch(mockCallCluster as any); + result = await collectors.bulkFetch(mockCallCluster); } catch (err) { // Do nothing } @@ -111,7 +128,7 @@ describe('CollectorSet', () => { }) ); - const result = await collectors.bulkFetch(mockCallCluster as any); + const result = await collectors.bulkFetch(mockCallCluster); expect(result).toStrictEqual([ { type: 'MY_TEST_COLLECTOR', @@ -129,7 +146,7 @@ describe('CollectorSet', () => { } as any) ); - const result = await collectors.bulkFetch(mockCallCluster as any); + const result = await collectors.bulkFetch(mockCallCluster); expect(result).toStrictEqual([ { type: 'MY_TEST_COLLECTOR', @@ -152,7 +169,7 @@ describe('CollectorSet', () => { }) ); - const result = await collectors.bulkFetch(mockCallCluster as any); + const result = await collectors.bulkFetch(mockCallCluster); expect(result).toStrictEqual([ { type: 'MY_TEST_COLLECTOR', diff --git a/src/plugins/usage_collection/server/collector/collector_set.ts b/src/plugins/usage_collection/server/collector/collector_set.ts index b6308d66033885..2b60a45c4065dc 100644 --- a/src/plugins/usage_collection/server/collector/collector_set.ts +++ b/src/plugins/usage_collection/server/collector/collector_set.ts @@ -32,10 +32,10 @@ export class CollectorSet { private _waitingForAllCollectorsTimestamp?: number; private readonly logger: Logger; private readonly maximumWaitTimeForAllCollectorsInS: number; - private collectors: Array> = []; + private readonly collectors: Map>; constructor({ logger, maximumWaitTimeForAllCollectorsInS, collectors = [] }: CollectorSetConfig) { this.logger = logger; - this.collectors = collectors; + this.collectors = new Map(collectors.map((collector) => [collector.type, collector])); this.maximumWaitTimeForAllCollectorsInS = maximumWaitTimeForAllCollectorsInS || 60; } @@ -55,7 +55,11 @@ export class CollectorSet { throw new Error('CollectorSet can only have Collector instances registered'); } - this.collectors.push(collector); + if (this.collectors.get(collector.type)) { + throw new Error(`Usage collector's type "${collector.type}" is duplicated.`); + } + + this.collectors.set(collector.type, collector); if (collector.init) { this.logger.debug(`Initializing ${collector.type} collector`); @@ -64,7 +68,7 @@ export class CollectorSet { }; public getCollectorByType = (type: string) => { - return this.collectors.find((c) => c.type === type); + return [...this.collectors.values()].find((c) => c.type === type); }; public isUsageCollector = (x: UsageCollector | any): x is UsageCollector => { @@ -81,7 +85,7 @@ export class CollectorSet { const collectorTypesNotReady: string[] = []; let allReady = true; - for (const collector of collectorSet.collectors) { + for (const collector of collectorSet.collectors.values()) { if (!(await collector.isReady())) { allReady = false; collectorTypesNotReady.push(collector.type); @@ -113,10 +117,10 @@ export class CollectorSet { public bulkFetch = async ( callCluster: LegacyAPICaller, - collectors: Array> = this.collectors + collectors: Map> = this.collectors ) => { const responses = []; - for (const collector of collectors) { + for (const collector of collectors.values()) { this.logger.debug(`Fetching data from ${collector.type} collector`); try { responses.push({ @@ -136,7 +140,7 @@ export class CollectorSet { * @return {new CollectorSet} */ public getFilteredCollectorSet = (filter: (col: Collector) => boolean) => { - const filtered = this.collectors.filter(filter); + const filtered = [...this.collectors.values()].filter(filter); return this.makeCollectorSetFromArray(filtered); }; @@ -188,12 +192,12 @@ export class CollectorSet { // TODO: remove public map = (mapFn: any) => { - return this.collectors.map(mapFn); + return [...this.collectors.values()].map(mapFn); }; // TODO: remove public some = (someFn: any) => { - return this.collectors.some(someFn); + return [...this.collectors.values()].some(someFn); }; private makeCollectorSetFromArray = (collectors: Collector[]) => {