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

[Telemetry] Report Kibana distro in local collectors + Usage Collectors in TS #55859

Merged
merged 6 commits into from
Feb 5, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ describe('telemetry_usage_collector', () => {
const collectorOptions = createTelemetryUsageCollector(usageCollector, server);

expect(collectorOptions.type).toBe('static_telemetry');
expect(await collectorOptions.fetch()).toEqual(expectedObject);
expect(await collectorOptions.fetch({} as any)).toEqual(expectedObject); // Sending any as the callCluster client because it's not needed in this collector but TS requires it when calling it.
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,27 @@
* under the License.
*/

import { get, omit } from 'lodash';
import { omit } from 'lodash';
import { UsageCollectionSetup } from 'src/plugins/usage_collection/server';
import { CallCluster } from 'src/legacy/core_plugins/elasticsearch';

export function handleKibanaStats(server, response) {
export interface KibanaUsageStats {
kibana: {
index: string;
};
kibana_stats: {
os: {
platform: string;
platformRelease: string;
distro?: string;
distroRelease?: string;
};
};

[plugin: string]: any;
}

export function handleKibanaStats(server: any, response?: KibanaUsageStats) {
if (!response) {
server.log(
['warning', 'telemetry', 'local-stats'],
Expand All @@ -30,8 +48,17 @@ export function handleKibanaStats(server, response) {

const { kibana, kibana_stats: kibanaStats, ...plugins } = response;

const platform = get(kibanaStats, 'os.platform', 'unknown');
const platformRelease = get(kibanaStats, 'os.platformRelease', 'unknown');
const os = {
platform: 'unknown',
platformRelease: 'unknown',
...kibanaStats.os,
};
const formattedOsStats = Object.entries(os).reduce((acc, [key, value]) => {
return {
...acc,
[`${key}s`]: [{ [key]: value, count: 1 }],
};
}, {});
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure i understand the difference between the previous code and the current one?

Copy link
Member Author

Choose a reason for hiding this comment

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

The previous code was only mapping os.platforms[].platform and os.platformReleases[].platformRelease.

Now we are copying every element in kibanaStats.os (also including distro and distroRelease), but following the convention of os[keyName + "s"] = [ { [keyName]: value, count: 1 } ] (os[KEY_IN_PLURAL] = Array<{ [KEY]: string, count: number }>).

I can try changing it to something more strict and map the 2 new missing fields (distro and distroRelease), but that will result in the next time we add something to the os payload, we'll face this same issue again.


const version = server
.config()
Expand All @@ -44,16 +71,16 @@ export function handleKibanaStats(server, response) {
...omit(kibana, 'index'), // discard index
count: 1,
indices: 1,
os: {
platforms: [{ platform, count: 1 }],
platformReleases: [{ platformRelease, count: 1 }],
},
os: formattedOsStats,
versions: [{ version, count: 1 }],
plugins,
};
}

export async function getKibana(usageCollection, callWithInternalUser) {
export async function getKibana(
usageCollection: UsageCollectionSetup,
callWithInternalUser: CallCluster
): Promise<KibanaUsageStats> {
const usage = await usageCollection.bulkFetch(callWithInternalUser);
return usageCollection.toObject(usage);
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,25 @@ import { get, omit } from 'lodash';
import { getClusterInfo } from './get_cluster_info';
import { getClusterStats } from './get_cluster_stats';
// @ts-ignore
import { getKibana, handleKibanaStats } from './get_kibana';
import { getKibana, handleKibanaStats, KibanaUsageStats } from './get_kibana';
import { StatsGetter } from '../collection_manager';

/**
* Handle the separate local calls by combining them into a single object response that looks like the
* "cluster_stats" document from X-Pack monitoring.
*
* @param {Object} server ??
* @param {Object} clusterInfo Cluster info (GET /)
* @param {Object} clusterStats Cluster stats (GET /_cluster/stats)
* @param {Object} kibana The Kibana Usage stats
* @return {Object} A combined object containing the different responses.
*/
export function handleLocalStats(server: any, clusterInfo: any, clusterStats: any, kibana: any) {
export function handleLocalStats(
server: any,
clusterInfo: any,
clusterStats: any,
kibana: KibanaUsageStats
) {
return {
timestamp: new Date().toISOString(),
cluster_uuid: get(clusterInfo, 'cluster_uuid'),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,30 @@
* under the License.
*/

export class Collector {
import { Logger } from 'kibana/server';
import { CallCluster } from 'src/legacy/core_plugins/elasticsearch';

export type CollectorFormatForBulkUpload<T, U> = (result: T) => { type: string; payload: U };

export interface CollectorOptions<T = unknown, U = T> {
type: string;
init?: Function;
fetch: (callCluster: CallCluster) => Promise<T> | T;
Copy link
Member

Choose a reason for hiding this comment

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

I believe callCluster should be optional

  fetch: (callCluster?: CallCluster) => Promise<T> | T;

There is no need to pass generic T at the interface level. We do not need the fetch return type when registering the collector. It could be scoped to fetch or simply the use of ..) => unknown is enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe callCluster should be optional

If we do it optional, every fetch will need to check if callCluster is not undefined before using it, but from the CollectorSet we are always providing it (and we should be enforced to provide it).

Declaring your fetch method without any argument if not using the callCluster parameter is still fine for TS (and even encouraged by eslint). A quick TS-safe example I just tested below:

  usageCollection.makeUsageCollector({
    type: KIBANA_REPORTING_TYPE,
    isReady,
    fetch: () => ({ count: 1 }),
  });

There is no need to pass generic T at the interface level. We do not need the fetch return type when registering the collector. It could be scoped to fetch or simply the use of ..) => unknown is enough.

The reason for being at the interface level is so formatForBulkUpload infers the type from the output of fetch. And it also will fail to compile if formatForBulkUpload is assuming an input different to the output of fetch.

An example is x-pack/legacy/plugins/reporting/server/usage/reporting_usage_collector.ts. You can see there is no need to specify the types when calling makeUsageCollector as it infers it automagically from the methods definitions.

/*
* A hook for allowing the fetched data payload to be organized into a typed
* data model for internal bulk upload. See defaultFormatterForBulkUpload for
* a generic example.
*/
formatForBulkUpload?: CollectorFormatForBulkUpload<T, U>;
isReady: () => Promise<boolean> | boolean;
}

export class Collector<T = unknown, U = T> {
public readonly type: CollectorOptions<T, U>['type'];
public readonly init?: CollectorOptions<T, U>['init'];
public readonly fetch: CollectorOptions<T, U>['fetch'];
private readonly _formatForBulkUpload?: CollectorFormatForBulkUpload<T, U>;
public readonly isReady: CollectorOptions<T, U>['isReady'];
/*
* @param {Object} logger - logger object
* @param {String} options.type - property name as the key for the data
Expand All @@ -27,8 +50,8 @@ export class Collector {
* @param {Function} options.rest - optional other properties
*/
constructor(
logger,
{ type, init, fetch, formatForBulkUpload = null, isReady = null, ...options } = {}
protected readonly log: Logger,
{ type, init, fetch, formatForBulkUpload, isReady, ...options }: CollectorOptions<T, U>
) {
if (type === undefined) {
throw new Error('Collector must be instantiated with a options.type string property');
Expand All @@ -42,41 +65,27 @@ export class Collector {
throw new Error('Collector must be instantiated with a options.fetch function property');
}

this.log = logger;

Object.assign(this, options); // spread in other properties and mutate "this"

this.type = type;
this.init = init;
this.fetch = fetch;

const defaultFormatterForBulkUpload = result => ({ type, payload: result });
this._formatForBulkUpload = formatForBulkUpload || defaultFormatterForBulkUpload;
if (typeof isReady === 'function') {
Copy link
Member

Choose a reason for hiding this comment

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

Lets keep this typeof check. In case someone sets isReady to true the collectorSet will not throw when attempting to execute something other than a function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure! I was relying on TS to ensure users will only provide a function here, but I'll add the safety check just in case anyone does it in a .js or as any cast

this.isReady = isReady;
}
this.isReady = isReady;
this._formatForBulkUpload = formatForBulkUpload;
}

/*
* @param {Function} callCluster - callCluster function
*/
fetchInternal(callCluster) {
if (typeof callCluster !== 'function') {
throw new Error('A `callCluster` function must be passed to the fetch methods of collectors');
public formatForBulkUpload(result: T) {
if (this._formatForBulkUpload) {
return this._formatForBulkUpload(result);
} else {
return this.defaultFormatterForBulkUpload(result);
}
return this.fetch(callCluster);
}

/*
* A hook for allowing the fetched data payload to be organized into a typed
* data model for internal bulk upload. See defaultFormatterForBulkUpload for
* a generic example.
*/
formatForBulkUpload(result) {
return this._formatForBulkUpload(result);
}

isReady() {
throw `isReady() must be implemented in ${this.type} collector`;
protected defaultFormatterForBulkUpload(result: T) {
return {
type: this.type,
payload: result,
};
}
}
25 changes: 11 additions & 14 deletions src/plugins/usage_collection/server/collector/collector_set.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,39 +20,37 @@
import { snakeCase } from 'lodash';
import { Logger } from 'kibana/server';
import { CallCluster } from 'src/legacy/core_plugins/elasticsearch';
// @ts-ignore
import { Collector } from './collector';
// @ts-ignore
import { Collector, CollectorOptions } from './collector';
import { UsageCollector } from './usage_collector';

interface CollectorSetConfig {
logger: Logger;
maximumWaitTimeForAllCollectorsInS: number;
collectors?: Collector[];
collectors?: Array<Collector<any, any>>;
}

export class CollectorSet {
private _waitingForAllCollectorsTimestamp?: number;
private logger: Logger;
private readonly maximumWaitTimeForAllCollectorsInS: number;
private collectors: Collector[] = [];
private collectors: Array<Collector<any, any>> = [];
constructor({ logger, maximumWaitTimeForAllCollectorsInS, collectors = [] }: CollectorSetConfig) {
this.logger = logger;
this.collectors = collectors;
this.maximumWaitTimeForAllCollectorsInS = maximumWaitTimeForAllCollectorsInS || 60;
}

public makeStatsCollector = (options: any) => {
public makeStatsCollector = <T, U>(options: CollectorOptions<T, U>) => {
return new Collector(this.logger, options);
};
public makeUsageCollector = (options: any) => {
public makeUsageCollector = <T, U>(options: CollectorOptions<T, U>) => {
return new UsageCollector(this.logger, options);
};

/*
* @param collector {Collector} collector object
*/
public registerCollector = (collector: Collector) => {
public registerCollector = <T, U>(collector: Collector<T, U>) => {
// check instanceof
if (!(collector instanceof Collector)) {
throw new Error('CollectorSet can only have Collector instances registered');
Expand Down Expand Up @@ -115,15 +113,15 @@ export class CollectorSet {

public bulkFetch = async (
callCluster: CallCluster,
collectors: Collector[] = this.collectors
collectors: Array<Collector<any, any>> = this.collectors
) => {
const responses = [];
for (const collector of collectors) {
this.logger.debug(`Fetching data from ${collector.type} collector`);
try {
responses.push({
type: collector.type,
result: await collector.fetchInternal(callCluster),
result: await collector.fetch(callCluster),
});
} catch (err) {
this.logger.warn(err);
Expand All @@ -148,14 +146,13 @@ export class CollectorSet {
};

// convert an array of fetched stats results into key/object
public toObject = (statsData: any) => {
if (!statsData) return {};
return statsData.reduce((accumulatedStats: any, { type, result }: any) => {
public toObject = <Result, T>(statsData: Array<{ type: string; result: T }> = []) => {
return statsData.reduce<Result>((accumulatedStats, { type, result }) => {
return {
...accumulatedStats,
[type]: result,
};
}, {});
}, {} as Result);
};

// rename fields to use api conventions
Expand Down
2 changes: 0 additions & 2 deletions src/plugins/usage_collection/server/collector/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,5 @@
*/

export { CollectorSet } from './collector_set';
// @ts-ignore
export { Collector } from './collector';
// @ts-ignore
export { UsageCollector } from './usage_collector';
51 changes: 0 additions & 51 deletions src/plugins/usage_collection/server/collector/usage_collector.js

This file was deleted.

37 changes: 37 additions & 0 deletions src/plugins/usage_collection/server/collector/usage_collector.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { KIBANA_STATS_TYPE } from '../../common/constants';
import { Collector } from './collector';

export class UsageCollector<T = unknown, U = { usage: { [key: string]: T } }> extends Collector<
T,
U
> {
protected defaultUsageFormatterForBulkUpload(result: T) {
return {
type: KIBANA_STATS_TYPE,
payload: {
usage: {
[this.type]: result,
},
},
};
}
}
Loading