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

[UsageCollection] Expose KibanaRequest to explicitly opted-in collectors #83413

Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 1 addition & 1 deletion src/fixtures/telemetry_collectors/nested_collector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ interface Usage {
}

export class NestedInside {
collector?: UsageCollector<Usage, Usage>;
collector?: UsageCollector<Usage>;
createMyCollector() {
this.collector = collectorSet.makeUsageCollector<Usage>({
type: 'my_nested_collector',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

import { savedObjectsRepositoryMock, loggingSystemMock } from '../../../../../core/server/mocks';
import {
CollectorOptions,
Collector,
createUsageCollectionSetupMock,
} from '../../../../usage_collection/server/usage_collection.mock';

Expand All @@ -40,11 +40,11 @@ describe('telemetry_application_usage', () => {

const logger = loggingSystemMock.createLogger();

let collector: CollectorOptions;
let collector: Collector<unknown, unknown>;

const usageCollectionMock = createUsageCollectionSetupMock();
usageCollectionMock.makeUsageCollector.mockImplementation((config) => {
collector = config;
collector = new Collector(logger, config);
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is required because TS would complain about fetch not having the this context

return createUsageCollectionSetupMock().makeUsageCollector(config);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,22 @@
*/

import {
CollectorOptions,
Collector,
createUsageCollectionSetupMock,
} from '../../../../usage_collection/server/usage_collection.mock';
import { createCollectorFetchContextMock } from 'src/plugins/usage_collection/server/mocks';
import { registerCoreUsageCollector } from '.';
import { coreUsageDataServiceMock } from '../../../../../core/server/mocks';
import { coreUsageDataServiceMock, loggingSystemMock } from '../../../../../core/server/mocks';
import { CoreUsageData } from 'src/core/server/';

const logger = loggingSystemMock.createLogger();

describe('telemetry_core', () => {
let collector: CollectorOptions;
let collector: Collector<unknown, unknown>;

const usageCollectionMock = createUsageCollectionSetupMock();
usageCollectionMock.makeUsageCollector.mockImplementation((config) => {
collector = config;
collector = new Collector(logger, config);
return createUsageCollectionSetupMock().makeUsageCollector(config);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,13 @@

import { CspConfig, ICspConfig } from '../../../../../core/server';
import { createCspCollector } from './csp_collector';
import { httpServiceMock } from '../../../../../core/server/mocks';
import { createCollectorFetchContextMock } from 'src/plugins/usage_collection/server/mocks';
import { httpServiceMock, loggingSystemMock } from '../../../../../core/server/mocks';
import {
Collector,
createCollectorFetchContextMock,
} from 'src/plugins/usage_collection/server/mocks';

const logger = loggingSystemMock.createLogger();

describe('csp collector', () => {
let httpMock: ReturnType<typeof httpServiceMock.createSetupContract>;
Expand All @@ -36,7 +41,7 @@ describe('csp collector', () => {
});

test('fetches whether strict mode is enabled', async () => {
const collector = createCspCollector(httpMock);
const collector = new Collector(logger, createCspCollector(httpMock));

expect((await collector.fetch(mockedFetchContext)).strict).toEqual(true);

Expand All @@ -45,7 +50,7 @@ describe('csp collector', () => {
});

test('fetches whether the legacy browser warning is enabled', async () => {
const collector = createCspCollector(httpMock);
const collector = new Collector(logger, createCspCollector(httpMock));

expect((await collector.fetch(mockedFetchContext)).warnLegacyBrowsers).toEqual(true);

Expand All @@ -54,7 +59,7 @@ describe('csp collector', () => {
});

test('fetches whether the csp rules have been changed or not', async () => {
const collector = createCspCollector(httpMock);
const collector = new Collector(logger, createCspCollector(httpMock));

expect((await collector.fetch(mockedFetchContext)).rulesChangedFromDefault).toEqual(false);

Expand All @@ -63,7 +68,7 @@ describe('csp collector', () => {
});

test('does not include raw csp rules under any property names', async () => {
const collector = createCspCollector(httpMock);
const collector = new Collector(logger, createCspCollector(httpMock));

// It's important that we do not send the value of csp.rules here as it
// can be customized with values that can be identifiable to given
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,20 +17,25 @@
* under the License.
*/

import { pluginInitializerContextConfigMock } from '../../../../../core/server/mocks';
import {
CollectorOptions,
loggingSystemMock,
pluginInitializerContextConfigMock,
} from '../../../../../core/server/mocks';
import {
Collector,
createUsageCollectionSetupMock,
} from '../../../../usage_collection/server/usage_collection.mock';
import { createCollectorFetchContextMock } from '../../../../usage_collection/server/mocks';
import { registerKibanaUsageCollector } from './';

const logger = loggingSystemMock.createLogger();

describe('telemetry_kibana', () => {
let collector: CollectorOptions;
let collector: Collector<unknown, unknown>;

const usageCollectionMock = createUsageCollectionSetupMock();
usageCollectionMock.makeUsageCollector.mockImplementation((config) => {
collector = config;
collector = new Collector(logger, config);
return createUsageCollectionSetupMock().makeUsageCollector(config);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,23 @@
* under the License.
*/

import { uiSettingsServiceMock } from '../../../../../core/server/mocks';
import { loggingSystemMock, uiSettingsServiceMock } from '../../../../../core/server/mocks';
import {
CollectorOptions,
Collector,
createUsageCollectionSetupMock,
createCollectorFetchContextMock,
} from '../../../../usage_collection/server/usage_collection.mock';

import { registerManagementUsageCollector } from './';

const logger = loggingSystemMock.createLogger();

describe('telemetry_application_usage_collector', () => {
let collector: CollectorOptions;
let collector: Collector<unknown, unknown>;

const usageCollectionMock = createUsageCollectionSetupMock();
usageCollectionMock.makeUsageCollector.mockImplementation((config) => {
collector = config;
collector = new Collector(logger, config);
return createUsageCollectionSetupMock().makeUsageCollector(config);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,20 +19,23 @@

import { Subject } from 'rxjs';
import {
CollectorOptions,
Collector,
createUsageCollectionSetupMock,
createCollectorFetchContextMock,
} from '../../../../usage_collection/server/usage_collection.mock';

import { registerOpsStatsCollector } from './';
import { OpsMetrics } from '../../../../../core/server';
import { loggingSystemMock } from '../../../../../core/server/mocks';

const logger = loggingSystemMock.createLogger();

describe('telemetry_ops_stats', () => {
let collector: CollectorOptions;
let collector: Collector<unknown, unknown>;

const usageCollectionMock = createUsageCollectionSetupMock();
usageCollectionMock.makeStatsCollector.mockImplementation((config) => {
collector = config;
collector = new Collector(logger, config);
return createUsageCollectionSetupMock().makeStatsCollector(config);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,23 @@
* under the License.
*/

import { savedObjectsRepositoryMock } from '../../../../../core/server/mocks';
import { loggingSystemMock, savedObjectsRepositoryMock } from '../../../../../core/server/mocks';
import {
CollectorOptions,
Collector,
createUsageCollectionSetupMock,
createCollectorFetchContextMock,
} from '../../../../usage_collection/server/usage_collection.mock';

import { registerUiMetricUsageCollector } from './';

const logger = loggingSystemMock.createLogger();

describe('telemetry_ui_metric', () => {
let collector: CollectorOptions;
let collector: Collector<unknown, unknown>;

const usageCollectionMock = createUsageCollectionSetupMock();
usageCollectionMock.makeUsageCollector.mockImplementation((config) => {
collector = config;
collector = new Collector(logger, config);
return createUsageCollectionSetupMock().makeUsageCollector(config);
});

Expand Down
11 changes: 9 additions & 2 deletions src/plugins/telemetry/server/telemetry_collection/get_kibana.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { omit } from 'lodash';
import { UsageCollectionSetup } from 'src/plugins/usage_collection/server';
import {
ISavedObjectsRepository,
KibanaRequest,
LegacyAPICaller,
SavedObjectsClientContract,
} from 'kibana/server';
Expand Down Expand Up @@ -89,8 +90,14 @@ export async function getKibana(
usageCollection: UsageCollectionSetup,
callWithInternalUser: LegacyAPICaller,
asInternalUser: ElasticsearchClient,
soClient: SavedObjectsClientContract | ISavedObjectsRepository
soClient: SavedObjectsClientContract | ISavedObjectsRepository,
kibanaRequest: KibanaRequest | undefined // intentionally `| undefined` to enforce providing the parameter
): Promise<KibanaUsageStats> {
const usage = await usageCollection.bulkFetch(callWithInternalUser, asInternalUser, soClient);
const usage = await usageCollection.bulkFetch(
callWithInternalUser,
asInternalUser,
soClient,
kibanaRequest
);
return usageCollection.toObject(usage);
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import {
usageCollectionPluginMock,
createCollectorFetchContextMock,
} from '../../../usage_collection/server/mocks';
import { elasticsearchServiceMock } from '../../../../../src/core/server/mocks';
import { elasticsearchServiceMock, httpServerMock } from '../../../../../src/core/server/mocks';

function mockUsageCollection(kibanaUsage = {}) {
const usageCollection = usageCollectionPluginMock.createSetupContract();
Expand Down Expand Up @@ -87,6 +87,7 @@ function mockStatsCollectionConfig(clusterInfo: any, clusterStats: any, kibana:
...createCollectorFetchContextMock(),
esClient: mockGetLocalStats(clusterInfo, clusterStats),
usageCollection: mockUsageCollection(kibana),
kibanaRequest: httpServerMock.createKibanaRequest(),
timestamp: Date.now(),
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,15 +71,15 @@ export const getLocalStats: StatsGetter<{}, TelemetryLocalStats> = async (
config, // contains the new esClient already scoped contains usageCollection, callCluster, esClient, start, end and the saved objects client scoped to the request or the internal repository
afharo marked this conversation as resolved.
Show resolved Hide resolved
context // StatsCollectionContext contains logger and version (string)
) => {
const { callCluster, usageCollection, esClient, soClient } = config;
const { callCluster, usageCollection, esClient, soClient, kibanaRequest } = config;

return await Promise.all(
clustersDetails.map(async (clustersDetail) => {
const [clusterInfo, clusterStats, nodesUsage, kibana, dataTelemetry] = await Promise.all([
getClusterInfo(esClient), // cluster info
getClusterStats(esClient), // cluster stats (not to be confused with cluster _state_)
getNodesUsage(esClient), // nodes_usage info
getKibana(usageCollection, callCluster, esClient, soClient),
getKibana(usageCollection, callCluster, esClient, soClient, kibanaRequest),
getDataTelemetry(esClient),
]);
return handleLocalStats(
Expand Down
5 changes: 4 additions & 1 deletion src/plugins/telemetry_collection_manager/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,10 @@ export class TelemetryCollectionManagerPlugin
const soClient = config.unencrypted
? collectionSoService.getScopedClient(config.request)
: collectionSoService.createInternalRepository();
return { callCluster, timestamp, usageCollection, esClient, soClient };
// Provide the kibanaRequest so opted-in plugins can scope their custom clients only if the request is not encrypted
const kibanaRequest = config.unencrypted ? request : void 0;
Copy link
Member

Choose a reason for hiding this comment

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

why void 0 instead of undefined?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's essentially the same. But JS allows you do do let undefined = 1, while void 0 is univoquely undefined :)


return { callCluster, timestamp, usageCollection, esClient, soClient, kibanaRequest };
}

private async getOptInStats(optInStatus: boolean, config: StatsGetterConfig) {
Expand Down
1 change: 1 addition & 0 deletions src/plugins/telemetry_collection_manager/server/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ export interface StatsCollectionConfig {
timestamp: number;
esClient: ElasticsearchClient;
soClient: SavedObjectsClientContract | ISavedObjectsRepository;
kibanaRequest: KibanaRequest | undefined; // intentionally `| undefined` to enforce providing the parameter
}

export interface BasicStatsPayload {
Expand Down
4 changes: 2 additions & 2 deletions src/plugins/usage_collection/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,8 @@ Some background:

- `isReady` (added in v7.2.0 and v6.8.4) is a way for a usage collector to announce that some async process must finish first before it can return data in the `fetch` method (e.g. a client needs to ne initialized, or the task manager needs to run a task first). If any collector reports that it is not ready when we call its `fetch` method, we reset a flag to try again and, after a set amount of time, collect data from those collectors that are ready and skip any that are not. This means that if a collector returns `true` for `isReady` and it actually isn't ready to return data, there won't be telemetry data from that collector in that telemetry report (usually once per day). You should consider what it means if your collector doesn't return data in the first few documents when Kibana starts or, if we should wait for any other reason (e.g. the task manager needs to run your task first). If you need to tell telemetry collection to wait, you should implement this function with custom logic. If your `fetch` method can run without the need of any previous dependencies, then you can return true for `isReady` as shown in the example below.

- 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` or `esClient` wraps `asCurrentUser`, where the request headers are expected to have read privilege on the entire `.kibana' index. The `fetch` method also exposes the saved objects client that will have the correct scope when the collectors' `fetch` method is called.
- The `fetch` method needs to support multiple contexts in which it is called. For example, when a user requests the example of what we collect in the **Kibana>Advanced Settings>Usage data** section, the clients provided in the context of the function (`CollectorFetchContext`) are scoped to that user's privileges. The reason is to avoid exposing via telemetry any data that user should not have access to (i.e.: if the user does not have access to certain indices, they shouldn't be allowed to see the number of documents that exists in it). In this case, the `fetch` method receives the clients `callCluster`, `esClient` and `soClient` scoped to the user who performed the HTTP API request. Alternatively, when requesting the usage data to be reported to the Remote Telemetry Service, the clients are scoped to the internal Kibana user (`kibana_system`). Please, mind it might have lower-level access than the default super-admin `elastic` test user.
In some scenarios, your collector might need to maintain its own client. An example of that is the `monitoring` plugin, that maintains a connection to the Remote Monitoring Cluster to push its monitoring data. If that's the case, your plugin can opt-in to receive the additional `kibanaRequest` parameter by adding `extendFetchContext.kibanaRequest: true` to the collector's config: it will be appended to the context of the `fetch` method only if the request needs to be scoped to a user other than Kibana Internal, so beware that your collector will need to work for both scenarios (especially for the scenario when `kibanaRequest` is missing).

Note: there will be many cases where you won't need to use the `callCluster`, `esClient` or `soClient` 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.

Expand Down
Loading