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 licensed feature usage API #63549

Merged
merged 12 commits into from
Apr 28, 2020
Merged
6 changes: 4 additions & 2 deletions src/core/server/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
*/
import { of } from 'rxjs';
import { duration } from 'moment';
import { PluginInitializerContext, CoreSetup, CoreStart } from '.';
import { PluginInitializerContext, CoreSetup, CoreStart, StartServicesAccessor } from '.';
import { CspConfig } from './csp';
import { loggingServiceMock } from './logging/logging_service.mock';
import { elasticsearchServiceMock } from './elasticsearch/elasticsearch_service.mock';
Expand Down Expand Up @@ -100,7 +100,9 @@ function pluginInitializerContextMock<T>(config: T = {} as T) {
return mock;
}

type CoreSetupMockType = MockedKeys<CoreSetup> & jest.Mocked<Pick<CoreSetup, 'getStartServices'>>;
type CoreSetupMockType = MockedKeys<CoreSetup> & {
getStartServices: jest.MockedFunction<StartServicesAccessor<any, any>>;
};
Comment on lines -103 to +105
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I encountered (yet another) issue with the getStartServices mock type in tests. the mock StartServicesAccessor is now explicitly typed as any, any.


function createCoreSetupMock({
pluginStartDeps = {},
Expand Down
17 changes: 14 additions & 3 deletions x-pack/plugins/licensing/server/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,17 @@
* you may not use this file except in compliance with the Elastic License.
*/
import { BehaviorSubject } from 'rxjs';
import { LicensingPluginSetup } from './types';
import { LicensingPluginSetup, LicensingPluginStart } from './types';
import { licenseMock } from '../common/licensing.mock';
import { featureUsageMock } from './services/feature_usage_service.mock';

const createSetupMock = () => {
const createSetupMock = (): jest.Mocked<LicensingPluginSetup> => {
const license = licenseMock.createLicense();
const mock: jest.Mocked<LicensingPluginSetup> = {
const mock = {
license$: new BehaviorSubject(license),
refresh: jest.fn(),
createLicensePoller: jest.fn(),
featureUsage: featureUsageMock.createSetup(),
};
mock.refresh.mockResolvedValue(license);
mock.createLicensePoller.mockReturnValue({
Expand All @@ -23,7 +25,16 @@ const createSetupMock = () => {
return mock;
};

const createStartMock = (): jest.Mocked<LicensingPluginStart> => {
const mock = {
featureUsage: featureUsageMock.createStart(),
};

return mock;
};

export const licensingMock = {
createSetup: createSetupMock,
createStart: createStartMock,
...licenseMock,
};
17 changes: 12 additions & 5 deletions x-pack/plugins/licensing/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,13 @@ import {
} from 'src/core/server';

import { ILicense, PublicLicense, PublicFeatures } from '../common/types';
import { LicensingPluginSetup } from './types';
import { LicensingPluginSetup, LicensingPluginStart } from './types';
import { License } from '../common/license';
import { createLicenseUpdate } from '../common/license_update';

import { ElasticsearchError, RawLicense, RawFeatures } from './types';
import { registerRoutes } from './routes';
import { FeatureUsageService } from './services';

import { LicenseConfigType } from './licensing_config';
import { createRouteHandlerContext } from './licensing_route_handler_context';
Expand Down Expand Up @@ -77,18 +78,19 @@ function sign({
* A plugin for fetching, refreshing, and receiving information about the license for the
* current Kibana instance.
*/
export class LicensingPlugin implements Plugin<LicensingPluginSetup> {
export class LicensingPlugin implements Plugin<LicensingPluginSetup, LicensingPluginStart, {}, {}> {
private stop$ = new Subject();
private readonly logger: Logger;
private readonly config$: Observable<LicenseConfigType>;
private loggingSubscription?: Subscription;
private featureUsage = new FeatureUsageService();

constructor(private readonly context: PluginInitializerContext) {
this.logger = this.context.logger.get();
this.config$ = this.context.config.create<LicenseConfigType>();
}

public async setup(core: CoreSetup) {
public async setup(core: CoreSetup<{}, LicensingPluginStart>) {
this.logger.debug('Setting up Licensing plugin');
const config = await this.config$.pipe(take(1)).toPromise();
const pollingFrequency = config.api_polling_frequency;
Expand All @@ -101,13 +103,14 @@ export class LicensingPlugin implements Plugin<LicensingPluginSetup> {

core.http.registerRouteHandlerContext('licensing', createRouteHandlerContext(license$));

registerRoutes(core.http.createRouter());
registerRoutes(core.http.createRouter(), core.getStartServices);
core.http.registerOnPreResponse(createOnPreResponseHandler(refresh, license$));

return {
refresh,
license$,
createLicensePoller: this.createLicensePoller.bind(this),
featureUsage: this.featureUsage.setup(),
};
}

Expand Down Expand Up @@ -186,7 +189,11 @@ export class LicensingPlugin implements Plugin<LicensingPluginSetup> {
return error.message;
}

public async start(core: CoreStart) {}
public async start(core: CoreStart) {
return {
featureUsage: this.featureUsage.start(),
};
}

public stop() {
this.stop$.next();
Expand Down
30 changes: 30 additions & 0 deletions x-pack/plugins/licensing/server/routes/feature_usage.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import { IRouter, StartServicesAccessor } from 'src/core/server';
import { LicensingPluginStart } from '../types';

export function registerFeatureUsageRoute(
router: IRouter,
getStartServices: StartServicesAccessor<{}, LicensingPluginStart>
) {
router.get(
{ path: '/api/licensing/feature_usage', validate: false },
async (context, request, response) => {
const [, , { featureUsage }] = await getStartServices();
return response.ok({
body: [...featureUsage.getLastUsages().entries()].reduce(
(res, [featureName, lastUsage]) => {
return {
...res,
[featureName]: new Date(lastUsage).toISOString(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

toISOString returns the expected ISO 8601 format, so I did not see any reason to use moment instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

This may have already been discussed but I wonder if there could be any problems with using absolute dates. If clocks are not configured correctly, it could make making sense of this data harder from the Cloud side. One option could be to return a relative date, ie. how many seconds ago it was used.

Copy link
Contributor Author

@pgayvallet pgayvallet Apr 17, 2020

Choose a reason for hiding this comment

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

Same comment as #63549 (comment), not sure if we can change the expected format here.

One option could be to return a relative date, ie. how many seconds ago it was used

This has different drawbacks btw, such as being vulnerable to latency, and forcing to be processed on the fly or enhanced to not loose the time reference of the consumer call.

Also I would be expecting cloud instances to have properly synchronized clocks between their machines/vms/cris so this is probably a non-issue?

@kobelb?

Copy link
Contributor

Choose a reason for hiding this comment

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

Another problem could be that a user couldn't know in what timezone the timestamp was created. Not sure if it's a critical problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assumed it was not an issue, as the API consumption is just going to pool calls to the API to count the actual number of usages in a given period of time. TBH I would have returned a unix timestamp instead of an ISO date representation if the elasticsearch API counterpart was not returning this format.

If we want to have correct timezones, I can just change the feature usage service to store dates instead of unix timestamps, but once again, this is assuming the actual user/client and the kibana server are on the same timezone.

My main difficulty on this feature is that it's very unclear who has the power of decision on that API.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I would be expecting cloud instances to have properly synchronized clocks between their machines/vms/cris so this is probably a non-issue?

I think this is a safe enough assumption for its usage. There's always the potential for clock drift, but even if we're off a bit it won't have catastrophic effects.

Using an ISO-8601 format for dates is 👍 , they can be easily translated to a unix timestamp. Date::toISOString doesn't include the timezone offset, which is completely fine in this situation as the timezone that the server is running in is irrelevant.

};
},
{}
),
});
}
);
}
11 changes: 9 additions & 2 deletions x-pack/plugins/licensing/server/routes/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,16 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import { IRouter } from 'src/core/server';

import { IRouter, StartServicesAccessor } from 'src/core/server';
import { LicensingPluginStart } from '../types';
import { registerInfoRoute } from './info';
import { registerFeatureUsageRoute } from './feature_usage';

export function registerRoutes(router: IRouter) {
export function registerRoutes(
router: IRouter,
getStartServices: StartServicesAccessor<{}, LicensingPluginStart>
) {
registerInfoRoute(router);
registerFeatureUsageRoute(router, getStartServices);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import {
FeatureUsageService,
FeatureUsageServiceSetup,
FeatureUsageServiceStart,
} from './feature_usage_service';

const createSetupMock = (): jest.Mocked<FeatureUsageServiceSetup> => {
const mock = {
register: jest.fn(),
};

return mock;
};

const createStartMock = (): jest.Mocked<FeatureUsageServiceStart> => {
const mock = {
notifyUsage: jest.fn(),
getLastUsages: jest.fn(),
};

return mock;
};

const createServiceMock = (): jest.Mocked<PublicMethodsOf<FeatureUsageService>> => {
const mock = {
setup: jest.fn(() => createSetupMock()),
start: jest.fn(() => createStartMock()),
};

return mock;
};

export const featureUsageMock = {
create: createServiceMock,
createSetup: createSetupMock,
createStart: createStartMock,
};
116 changes: 116 additions & 0 deletions x-pack/plugins/licensing/server/services/feature_usage_service.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { FeatureUsageService } from './feature_usage_service';

describe('FeatureUsageService', () => {
let service: FeatureUsageService;

beforeEach(() => {
service = new FeatureUsageService();
});

afterEach(() => {
jest.restoreAllMocks();
});

const toObj = (map: ReadonlyMap<string, any>): Record<string, any> =>
Object.fromEntries(map.entries());

describe('#setup', () => {
describe('#register', () => {
it('throws when registering the same feature twice', () => {
const setup = service.setup();
setup.register('foo');
expect(() => {
setup.register('foo');
}).toThrowErrorMatchingInlineSnapshot(`"Feature 'foo' has already been registered."`);
});
});
});

describe('#start', () => {
describe('#notifyUsage', () => {
it('allows to notify a feature usage', () => {
const setup = service.setup();
setup.register('feature');
const start = service.start();
start.notifyUsage('feature', 127001);

expect(start.getLastUsages().get('feature')).toBe(127001);
});

it('can receive a Date object', () => {
const setup = service.setup();
setup.register('feature');
const start = service.start();

const usageTime = new Date(2015, 9, 21, 17, 54, 12);
start.notifyUsage('feature', usageTime);
expect(start.getLastUsages().get('feature')).toBe(usageTime.getTime());
});

it('uses the current time when `usedAt` is unspecified', () => {
jest.spyOn(Date, 'now').mockReturnValue(42);

const setup = service.setup();
setup.register('feature');
const start = service.start();
start.notifyUsage('feature');

expect(start.getLastUsages().get('feature')).toBe(42);
});

it('throws when notifying for an unregistered feature', () => {
service.setup();
const start = service.start();
expect(() => {
start.notifyUsage('unregistered');
}).toThrowErrorMatchingInlineSnapshot(`"Feature 'unregistered' is not registered."`);
});
});

describe('#getLastUsages', () => {
it('returns the last usage for all used features', () => {
const setup = service.setup();
setup.register('featureA');
setup.register('featureB');
const start = service.start();
start.notifyUsage('featureA', 127001);
start.notifyUsage('featureB', 6666);

expect(toObj(start.getLastUsages())).toEqual({
featureA: 127001,
featureB: 6666,
});
});

it('returns the last usage even after notifying for an older usage', () => {
const setup = service.setup();
setup.register('featureA');
const start = service.start();
start.notifyUsage('featureA', 1000);
start.notifyUsage('featureA', 500);

expect(toObj(start.getLastUsages())).toEqual({
featureA: 1000,
});
});

it('does not return entries for unused registered features', () => {
const setup = service.setup();
setup.register('featureA');
setup.register('featureB');
const start = service.start();
start.notifyUsage('featureA', 127001);

expect(toObj(start.getLastUsages())).toEqual({
featureA: 127001,
});
});
});
});
});
63 changes: 63 additions & 0 deletions x-pack/plugins/licensing/server/services/feature_usage_service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { isDate } from 'lodash';

/** @public */
export interface FeatureUsageServiceSetup {
/**
* Register a feature to be able to notify of it's usages using the {@link FeatureUsageServiceStart | service start contract}.
*/
register(featureName: string): void;
}

/** @public */
export interface FeatureUsageServiceStart {
/**
* Notify of a registered feature usage at given time.
*
* @param featureName - the name of the feature to notify usage of
* @param usedAt - Either a `Date` or an unix timestamp with ms. If not specified, it will be set to the current time.
*/
notifyUsage(featureName: string, usedAt?: Date | number): void;
/**
* Return a map containing last usage timestamp for all features.
* Features that were not used yet do not appear in the map.
*/
getLastUsages(): ReadonlyMap<string, number>;
}

export class FeatureUsageService {
private readonly features: string[] = [];
private readonly lastUsages = new Map<string, number>();

public setup(): FeatureUsageServiceSetup {
return {
register: featureName => {
if (this.features.includes(featureName)) {
throw new Error(`Feature '${featureName}' has already been registered.`);
}
this.features.push(featureName);
},
};
}

public start(): FeatureUsageServiceStart {
return {
notifyUsage: (featureName, usedAt = Date.now()) => {
if (!this.features.includes(featureName)) {
throw new Error(`Feature '${featureName}' is not registered.`);
}
if (isDate(usedAt)) {
usedAt = usedAt.getTime();
}
const currentValue = this.lastUsages.get(featureName) ?? 0;
this.lastUsages.set(featureName, Math.max(usedAt, currentValue));
},
getLastUsages: () => new Map(this.lastUsages.entries()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this return null values for features that were registered but have not been used? As it is right now, features that have not been used do not show up in the API response at all. I'm not sure what consumers of this expect, but it may be a bit clearer if all the tracked features show in this list, regardless of usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm following @rjernst 'specifications' from #60984 (comment) here. non-used features does not appear in this API apparently.

I'm unsure what liberty we have on that, as the goal seems to be mirroring ES' WIP api?

Copy link
Contributor

@mshustov mshustov Apr 17, 2020

Choose a reason for hiding this comment

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

why we need features list in this case at all? Can we removeregister call?

Copy link
Contributor

Choose a reason for hiding this comment

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

why we need features list in this case at all? Can we remove register call?

That was my first thought as well. If we don't need to return the list of all features (even if unused) then we could probably remove it. Though it does make it a bit safer that plugins can't just shove random strings into the notifyUsage API.

Maybe we could get away with a union string type for the allowed strings, though that would mean plugins have to update these types in order to add new usage tracking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why we need features list in this case at all? Can we remove register call?

There is no functional reasons for this call atm. I just added it because it felt more KP compliant for plugins to explicitly register every licensed feature they wanted to notify usages of. If we think this is useless, I can definitely remove that part.

Maybe we could get away with a union string type for the allowed strings

Mixed on that. In addition to that would mean plugins have to update these types in order to add new usage tracking, this also kinda breaks the feature for 3rd party plugins, as they wouldn't be able to register new features, even if realistically it's probably a non-issue.

Copy link
Contributor

@mshustov mshustov Apr 20, 2020

Choose a reason for hiding this comment

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

Though it does make it a bit safer that plugins can't just shove random strings into the notifyUsage API.

What prevents them from passing a random string to register as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing, but at least they can only notify usage of the arbitrary features that were registered.

};
}
}
Loading