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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
578defe
[Telemetry] Report Kibana distro in local collectors + Usage Collecto…
afharo Jan 24, 2020
153627a
Merge branch 'master' into telemetry/os-distro
elasticmachine Jan 27, 2020
4ef2ad3
Ensure isReady is a function
afharo Feb 4, 2020
18553e4
Move CollectorSet tests to TS + Jest
afharo Feb 4, 2020
c79009c
Merge branch 'master' of github.com:elastic/kibana into telemetry/os-…
afharo Feb 4, 2020
79b67c2
Fix test
afharo Feb 4, 2020
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
2 changes: 1 addition & 1 deletion src/legacy/core_plugins/telemetry/server/collectors/usage/telemetry_usage_collector.test.ts
View file
Open in desktop
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.
});
});
});
45 changes: 36 additions & 9 deletions ...server/telemetry_collection/get_kibana.js → ...server/telemetry_collection/get_kibana.ts
View file
Open in desktop
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);
}
11 changes: 9 additions & 2 deletions src/legacy/core_plugins/telemetry/server/telemetry_collection/get_local_stats.ts
View file
Open in desktop
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
67 changes: 38 additions & 29 deletions ..._collection/server/collector/collector.js → ..._collection/server/collector/collector.ts
View file
Open in desktop
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 = typeof isReady === 'function' ? isReady : () => true;
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,
};
}
}
Loading