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

Delay client-side feature usage registration until start #79365

Merged
merged 10 commits into from
Oct 7, 2020
1 change: 1 addition & 0 deletions test/scripts/jenkins_xpack_build_plugins.sh
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,6 @@ node scripts/build_kibana_platform_plugins \
--scan-dir "$XPACK_DIR/test/alerting_api_integration/plugins" \
--scan-dir "$XPACK_DIR/test/plugin_api_integration/plugins" \
--scan-dir "$XPACK_DIR/test/plugin_api_perf/plugins" \
--scan-dir "$XPACK_DIR/test/licensing_plugin/plugins" \
--workers 12 \
--verbose
2 changes: 1 addition & 1 deletion x-pack/plugins/licensing/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ export class LicensingPlugin implements Plugin<LicensingPluginSetup, LicensingPl
return {
refresh: refreshManually,
license$,
featureUsage: this.featureUsage.setup({ http: core.http }),
featureUsage: this.featureUsage.setup(),
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,33 @@ describe('FeatureUsageService', () => {

describe('#setup', () => {
describe('#register', () => {
it('calls the endpoint with the correct parameters', async () => {
const setup = service.setup({ http });
await setup.register('my-feature', 'platinum');
it('calls the endpoint on start with the correct parameters', async () => {
const setup = service.setup();
setup.register('my-feature', 'platinum');
setup.register('my-other-feature', 'gold');
expect(http.post).not.toHaveBeenCalled();

service.start({ http });
expect(http.post).toHaveBeenCalledTimes(1);
expect(http.post).toHaveBeenCalledWith('/internal/licensing/feature_usage/register', {
body: JSON.stringify({
featureName: 'my-feature',
licenseType: 'platinum',
}),
body: JSON.stringify([
{ featureName: 'my-feature', licenseType: 'platinum' },
{ featureName: 'my-other-feature', licenseType: 'gold' },
]),
});
});

it('does not call endpoint if on anonymous path', async () => {
it('does not call endpoint on start if no registrations', async () => {
service.setup();
service.start({ http });
expect(http.post).not.toHaveBeenCalled();
});

it('does not call endpoint on start if on anonymous path', async () => {
http.anonymousPaths.isAnonymous.mockReturnValue(true);
const setup = service.setup({ http });
await setup.register('my-feature', 'platinum');
const setup = service.setup();
setup.register('my-feature', 'platinum');
service.start({ http });
expect(http.post).not.toHaveBeenCalled();
});
});
Expand All @@ -42,7 +53,7 @@ describe('FeatureUsageService', () => {
describe('#start', () => {
describe('#notifyUsage', () => {
it('calls the endpoint with the correct parameters', async () => {
service.setup({ http });
service.setup();
const start = service.start({ http });
await start.notifyUsage('my-feature', 42);

Expand All @@ -56,7 +67,7 @@ describe('FeatureUsageService', () => {
});

it('correctly convert dates', async () => {
service.setup({ http });
service.setup();
const start = service.start({ http });

const now = new Date();
Expand All @@ -74,7 +85,7 @@ describe('FeatureUsageService', () => {

it('does not call endpoint if on anonymous path', async () => {
http.anonymousPaths.isAnonymous.mockReturnValue(true);
service.setup({ http });
service.setup();
const start = service.start({ http });
await start.notifyUsage('my-feature', 42);
expect(http.post).not.toHaveBeenCalled();
Expand Down
36 changes: 18 additions & 18 deletions x-pack/plugins/licensing/public/services/feature_usage_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,15 @@
*/

import { isDate } from 'lodash';
import type { HttpSetup, HttpStart } from 'src/core/public';
import type { HttpStart } from 'src/core/public';
import { LicenseType } from '../../common/types';

/** @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, licenseType: LicenseType): Promise<void>;
register(featureName: string, licenseType: LicenseType): void;
}

/** @public */
Expand All @@ -27,10 +27,6 @@ export interface FeatureUsageServiceStart {
notifyUsage(featureName: string, usedAt?: Date | number): Promise<void>;
}

interface SetupDeps {
http: HttpSetup;
}

interface StartDeps {
http: HttpStart;
}
Expand All @@ -39,29 +35,33 @@ interface StartDeps {
* @internal
*/
export class FeatureUsageService {
public setup({ http }: SetupDeps): FeatureUsageServiceSetup {
private readonly registrations: Array<{ featureName: string; licenseType: LicenseType }> = [];

public setup(): FeatureUsageServiceSetup {
return {
register: async (featureName, licenseType) => {
// Skip registration if on logged-out page
// NOTE: this only works because the login page does a full-page refresh after logging in
// If this is ever changed, this code will need to buffer registrations and call them after the user logs in.
if (http.anonymousPaths.isAnonymous(window.location.pathname)) return;

await http.post('/internal/licensing/feature_usage/register', {
body: JSON.stringify({
featureName,
licenseType,
}),
});
this.registrations.push({ featureName, licenseType });
},
};
}

public start({ http }: StartDeps): FeatureUsageServiceStart {
// Skip registration if on logged-out page
// NOTE: this only works because the login page does a full-page refresh after logging in
// If this is ever changed, this code will need to buffer registrations and call them after the user logs in.
const registrationPromise =
http.anonymousPaths.isAnonymous(window.location.pathname) || this.registrations.length === 0
? Promise.resolve()
: http.post('/internal/licensing/feature_usage/register', {
body: JSON.stringify(this.registrations),
});

return {
notifyUsage: async (featureName, usedAt = Date.now()) => {
// Skip notification if on logged-out page
if (http.anonymousPaths.isAnonymous(window.location.pathname)) return;
// Wait for registrations to complete
await registrationPromise;

const lastUsed = isDate(usedAt) ? usedAt.getTime() : usedAt;
await http.post('/internal/licensing/feature_usage/notify', {
Expand Down
28 changes: 16 additions & 12 deletions x-pack/plugins/licensing/server/routes/internal/register_feature.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,26 @@ export function registerRegisterFeatureRoute(
{
path: '/internal/licensing/feature_usage/register',
validate: {
body: schema.object({
featureName: schema.string(),
licenseType: schema.string({
validate: (value) => {
if (!(value in LICENSE_TYPE)) {
return `Invalid license type: ${value}`;
}
},
}),
}),
body: schema.arrayOf(
schema.object({
featureName: schema.string(),
licenseType: schema.string({
validate: (value) => {
if (!(value in LICENSE_TYPE)) {
return `Invalid license type: ${value}`;
}
},
}),
})
),
},
},
async (context, request, response) => {
const { featureName, licenseType } = request.body;
const registrations = request.body;

featureUsageSetup.register(featureName, licenseType as LicenseType);
registrations.forEach(({ featureName, licenseType }) => {
featureUsageSetup.register(featureName, licenseType as LicenseType);
});

return response.ok({
body: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,17 +158,7 @@ export class UiActionsServiceEnhancements

private registerFeatureUsage = (definition: ActionFactoryDefinition<any, any, any>): void => {
if (!definition.minimalLicense || !definition.licenseFeatureName) return;

// Intentionally don't wait for response because
// happens in setup phase and has to be sync
this.deps.featureUsageSetup
.register(definition.licenseFeatureName, definition.minimalLicense)
.catch(() => {
// eslint-disable-next-line no-console
console.warn(
`ActionFactory [actionFactory.id = ${definition.id}] fail to register feature for featureUsage.`
);
});
this.deps.featureUsageSetup.register(definition.licenseFeatureName, definition.minimalLicense);
};

public readonly telemetry = (state: DynamicActionsState, telemetry: Record<string, any> = {}) => {
Expand Down
1 change: 1 addition & 0 deletions x-pack/test/licensing_plugin/config.public.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export default async function ({ readConfigFile }: FtrConfigProviderContext) {
KIBANA_ROOT,
'test/plugin_functional/plugins/core_provider_plugin'
)}`,
`--plugin-path=${path.resolve(__dirname, 'plugins/test_feature_usage')}`,
],
},
};
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"id": "testFeatureUsage",
"version": "kibana",
"server": false,
"ui": true,
"requiredPlugins": ["licensing"]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/*
* 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 { TestFeatureUsagePlugin } from './plugin';

export const plugin = () => new TestFeatureUsagePlugin();
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* 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 { CoreSetup } from 'src/core/server';
import { LicensingPluginSetup } from '../../../../../plugins/licensing/public';

interface SetupPlugins {
licensing: LicensingPluginSetup;
}

export class TestFeatureUsagePlugin {
public setup(core: CoreSetup, plugins: SetupPlugins) {
plugins.licensing.featureUsage.register('test-client-A', 'gold');
plugins.licensing.featureUsage.register('test-client-B', 'enterprise');
}

public start() {}
public stop() {}
}
23 changes: 2 additions & 21 deletions x-pack/test/licensing_plugin/public/feature_usage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,7 @@
*/
import expect from '@kbn/expect';
import { FtrProviderContext } from '../services';
import {
LicensingPluginSetup,
LicensingPluginStart,
LicenseType,
} from '../../../plugins/licensing/public';
import { LicensingPluginStart, LicenseType } from '../../../plugins/licensing/public';
import '../../../../test/plugin_functional/plugins/core_provider_plugin/types';

interface FeatureUsage {
Expand All @@ -25,19 +21,6 @@ export default function (ftrContext: FtrProviderContext) {
const browser = getService('browser');
const PageObjects = getPageObjects(['common', 'security']);

const registerFeature = async (featureName: string, licenseType: LicenseType) => {
await browser.executeAsync(
async (feature, type, cb) => {
const { setup } = window._coreProvider;
const licensing: LicensingPluginSetup = setup.plugins.licensing;
await licensing.featureUsage.register(feature, type);
cb();
},
featureName,
licenseType
);
};

const notifyFeatureUsage = async (featureName: string, lastUsed: number) => {
await browser.executeAsync(
async (feature, time, cb) => {
Expand All @@ -57,12 +40,10 @@ export default function (ftrContext: FtrProviderContext) {
});

it('allows to register features to the server', async () => {
await registerFeature('test-client-A', 'gold');
await registerFeature('test-client-B', 'enterprise');

const response = await supertest.get('/api/licensing/feature_usage').expect(200);
const features = response.body.features.map(({ name }: FeatureUsage) => name);

// These were registered by the plugin in ../plugins/test_feature_usage
expect(features).to.contain('test-client-A');
expect(features).to.contain('test-client-B');
});
Expand Down