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

[Reporting/Server] register plugin routes synchronously #68976

Merged
merged 15 commits into from
Jun 15, 2020
Merged
54 changes: 29 additions & 25 deletions x-pack/plugins/reporting/server/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/

import * as Rx from 'rxjs';
import { first, map, mapTo } from 'rxjs/operators';
import { first, map, take } from 'rxjs/operators';
import {
BasePath,
ElasticsearchServiceSetup,
Expand Down Expand Up @@ -33,7 +33,8 @@ export interface ReportingInternalSetup {
security?: SecurityPluginSetup;
}

interface ReportingInternalStart {
export interface ReportingInternalStart {
browserDriverFactory: HeadlessChromiumDriverFactory;
enqueueJob: EnqueueJobFn;
esqueue: ESQueueInstance;
savedObjects: SavedObjectsServiceStart;
Expand All @@ -43,33 +44,39 @@ interface ReportingInternalStart {
export class ReportingCore {
private pluginSetupDeps?: ReportingInternalSetup;
private pluginStartDeps?: ReportingInternalStart;
private browserDriverFactory?: HeadlessChromiumDriverFactory;
Copy link
Member Author

Choose a reason for hiding this comment

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

moved browserDriverFactory to be a plugin start dep.

private readonly pluginSetup$ = new Rx.ReplaySubject<ReportingInternalSetup>();
private readonly pluginStart$ = new Rx.ReplaySubject<ReportingInternalStart>();
private readonly pluginSetup$ = new Rx.ReplaySubject<boolean>(); // for pluginHasSetup
private readonly pluginStart$ = new Rx.ReplaySubject<ReportingInternalStart>(); // for getPluginStartDeps
private exportTypesRegistry = getExportTypesRegistry();
private config?: ReportingConfig;

constructor(private config: ReportingConfig) {}
constructor() {}

public pluginSetup(reportingSetupDeps: ReportingInternalSetup) {
this.pluginSetupDeps = reportingSetupDeps;
this.pluginSetup$.next(reportingSetupDeps);
this.pluginSetup$.next(true);
}

public pluginStart(reportingStartDeps: ReportingInternalStart) {
this.pluginStart$.next(reportingStartDeps);
}

public pluginHasStarted(): Promise<boolean> {
return this.pluginStart$.pipe(first(), mapTo(true)).toPromise();
public async pluginIsSetup(): Promise<boolean> {
// use deps and config as a cached resolver
if (this.pluginSetupDeps && this.config) {
return true;
}
return await this.pluginSetup$.pipe(take(2)).toPromise(); // once for pluginSetupDeps (sync) and twice for config (async)
}

public async pluginHasStarted(): Promise<boolean> {
return await this.getPluginStartDeps().then(() => true);
}

public setBrowserDriverFactory(browserDriverFactory: HeadlessChromiumDriverFactory) {
this.browserDriverFactory = browserDriverFactory;
public setConfig(config: ReportingConfig) {
this.config = config;
this.pluginSetup$.next(true);
Copy link
Member Author

Choose a reason for hiding this comment

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

Full "plugin setup" now depends on 2 things: the pluginSetup deps, and the async config. That is why this.pluginSetup$.next is called twice in this file.

}

/*
* Internal module dependencies
*/
public getExportTypesRegistry() {
return this.exportTypesRegistry;
}
Expand All @@ -93,15 +100,16 @@ export class ReportingCore {
}

public getConfig(): ReportingConfig {
if (!this.config) {
throw new Error('Config is not yet initialized');
}
return this.config;
}

public getScreenshotsObservable(): ScreenshotsObservableFn {
const { browserDriverFactory } = this;
if (!browserDriverFactory) {
throw new Error(`"browserDriverFactory" dependency hasn't initialized yet`);
}
return screenshotsObservableFactory(this.config.get('capture'), browserDriverFactory);
public async getScreenshotsObservable(): Promise<ScreenshotsObservableFn> {
const config = this.getConfig();
const { browserDriverFactory } = await this.getPluginStartDeps();
return screenshotsObservableFactory(config.get('capture'), browserDriverFactory);
}

public getPluginSetupDeps() {
Expand All @@ -111,18 +119,14 @@ export class ReportingCore {
return this.pluginSetupDeps;
}

/*
* Outside dependencies
*/

private async getPluginStartDeps() {
if (this.pluginStartDeps) {
return this.pluginStartDeps;
}
return await this.pluginStart$.pipe(first()).toPromise();
}

public async getElasticsearchService() {
public getElasticsearchService() {
return this.getPluginSetupDeps().elasticsearch;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,23 @@
*/

import nodeCrypto from '@elastic/node-crypto';
import { IUiSettingsClient, ElasticsearchServiceSetup } from 'kibana/server';
// @ts-ignore
import Puid from 'puid';
import sinon from 'sinon';
import { ReportingConfig, ReportingCore } from '../../../';
import { fieldFormats, UI_SETTINGS } from '../../../../../../../src/plugins/data/server';
import {
CSV_QUOTE_VALUES_SETTING,
CSV_SEPARATOR_SETTING,
} from '../../../../../../../src/plugins/share/server';
import { CancellationToken } from '../../../../common';
import { CSV_BOM_CHARS } from '../../../../common/constants';
import { LevelLogger } from '../../../lib';
import { setFieldFormats } from '../../../services';
import { createMockReportingCore } from '../../../test_helpers';
import { JobDocPayloadDiscoverCsv } from '../types';
import { executeJobFactory } from './execute_job';
import {
CSV_SEPARATOR_SETTING,
CSV_QUOTE_VALUES_SETTING,
} from '../../../../../../../src/plugins/share/server';

const delay = (ms: number) => new Promise((resolve) => setTimeout(() => resolve(), ms));

Expand Down Expand Up @@ -48,8 +50,8 @@ describe('CSV Execute Job', function () {

let clusterStub: any;
let configGetStub: any;
let mockReportingConfig: any;
let mockReportingCore: any;
let mockReportingConfig: ReportingConfig;
let mockReportingCore: ReportingCore;
let callAsCurrentUserStub: any;
let cancellationToken: any;

Expand Down Expand Up @@ -78,9 +80,11 @@ describe('CSV Execute Job', function () {
mockReportingConfig = { get: configGetStub, kbnConfig: { get: configGetStub } };

mockReportingCore = await createMockReportingCore(mockReportingConfig);
mockReportingCore.getUiSettingsServiceFactory = () => Promise.resolve(mockUiSettingsClient);
mockReportingCore.getElasticsearchService = () => Promise.resolve(mockElasticsearch);
mockReportingCore.config = mockReportingConfig;
mockReportingCore.getUiSettingsServiceFactory = () =>
Promise.resolve((mockUiSettingsClient as unknown) as IUiSettingsClient);
mockReportingCore.getElasticsearchService = () =>
mockElasticsearch as ElasticsearchServiceSetup;
mockReportingCore.setConfig(mockReportingConfig);

cancellationToken = new CancellationToken();

Expand Down Expand Up @@ -995,7 +999,8 @@ describe('CSV Execute Job', function () {
let maxSizeReached: boolean;

beforeEach(async function () {
mockReportingCore.getUiSettingsServiceFactory = () => mockUiSettingsClient;
mockReportingCore.getUiSettingsServiceFactory = () =>
Promise.resolve((mockUiSettingsClient as unknown) as IUiSettingsClient);
configGetStub.withArgs('csv', 'maxSizeBytes').returns(18);

callAsCurrentUserStub.onFirstCall().returns({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export const executeJobFactory: ExecuteJobFactory<ESQueueWorkerExecuteFn<
job: JobDocPayloadDiscoverCsv,
cancellationToken: any
) {
const elasticsearch = await reporting.getElasticsearchService();
const elasticsearch = reporting.getElasticsearchService();
const jobLogger = logger.clone([jobId]);

const {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ export async function generateCsvSearch(
};

const config = reporting.getConfig();
const elasticsearch = await reporting.getElasticsearchService();
const elasticsearch = reporting.getElasticsearchService();
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'm considering deleting this helper method entirely since the dep can come from getPluginSetupDeps

Copy link
Contributor

Choose a reason for hiding this comment

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

++ on always decreasing API surface area

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 will leave it in for now, since it allows us to mock the elasticsearch service pretty well.

const { callAsCurrentUser } = elasticsearch.legacy.client.asScoped(req);
const callCluster = (...params: [string, object]) => callAsCurrentUser(...params);
const uiSettings = await getUiSettings(uiConfig);
Expand Down
12 changes: 5 additions & 7 deletions x-pack/plugins/reporting/server/lib/jobs_query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,9 @@

import { i18n } from '@kbn/i18n';
import { errors as elasticsearchErrors } from 'elasticsearch';
import { ElasticsearchServiceSetup } from 'kibana/server';
import { get } from 'lodash';
import { ReportingCore } from '../';
import { AuthenticatedUser } from '../../../security/server';
import { ReportingConfig } from '../';
import { JobSource } from '../types';

const esErrors = elasticsearchErrors as Record<string, any>;
Expand Down Expand Up @@ -42,11 +41,8 @@ interface CountAggResult {

const getUsername = (user: AuthenticatedUser | null) => (user ? user.username : false);

export function jobsQueryFactory(
config: ReportingConfig,
elasticsearch: ElasticsearchServiceSetup
) {
const index = config.get('index');
export function jobsQueryFactory(reportingCore: ReportingCore) {
const { elasticsearch } = reportingCore.getPluginSetupDeps();
const { callAsInternalUser } = elasticsearch.legacy.client;

function execQuery(queryType: string, body: QueryBody) {
Expand All @@ -60,6 +56,8 @@ export function jobsQueryFactory(
},
};

const config = reportingCore.getConfig();
Copy link
Member Author

@tsullivan tsullivan Jun 12, 2020

Choose a reason for hiding this comment

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

lazily resolving the config off of reportingCore when needed.

const index = config.get('index');
const query = {
index: `${index}-*`,
body: Object.assign(defaultBody[queryType] || {}, body),
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/reporting/server/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

jest.mock('./browsers/install', () => ({
installBrowser: jest.fn().mockImplementation(() => ({
binaryPath$: {
Expand Down
59 changes: 23 additions & 36 deletions x-pack/plugins/reporting/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
* you may not use this file except in compliance with the Elastic License.
*/

import * as Rx from 'rxjs';
import { CoreSetup, CoreStart, Plugin, PluginInitializerContext } from 'src/core/server';
import { ReportingCore } from './';
import { initializeBrowserDriverFactory } from './browsers';
Expand All @@ -19,43 +18,38 @@ export class ReportingPlugin
implements Plugin<ReportingSetup, ReportingStart, ReportingSetupDeps, ReportingStartDeps> {
private readonly initializerContext: PluginInitializerContext<ReportingConfigType>;
private logger: LevelLogger;
private reportingCore?: ReportingCore;

// Setup some observables for modules that need to await setup/start
public readonly setup$ = new Rx.Subject<boolean>();
public readonly start$ = new Rx.Subject<boolean>();
private reportingCore: ReportingCore;

constructor(context: PluginInitializerContext<ReportingConfigType>) {
this.logger = new LevelLogger(context.logger.get());
this.initializerContext = context;
this.reportingCore = new ReportingCore();
}

public setup(core: CoreSetup, plugins: ReportingSetupDeps) {
const { elasticsearch, http } = core;
const { licensing, security } = plugins;
const { initializerContext: initContext } = this;
const { initializerContext: initContext, reportingCore } = this;

const router = http.createRouter();
const basePath = http.basePath.get;

reportingCore.pluginSetup({
elasticsearch,
licensing,
basePath,
router,
security,
});

registerReportingUsageCollector(reportingCore, plugins);
registerRoutes(reportingCore, this.logger);

// async background setup
(async () => {
const config = await buildConfig(initContext, core, this.logger);
const reportingCore = new ReportingCore(config);

reportingCore.pluginSetup({
elasticsearch,
licensing,
basePath,
router,
security,
});

registerReportingUsageCollector(reportingCore, plugins);
registerRoutes(reportingCore, this.logger);
this.reportingCore = reportingCore;

reportingCore.setConfig(config);
this.logger.debug('Setup complete');
this.setup$.next(true);
})().catch((e) => {
this.logger.error(`Error in Reporting setup, reporting may not function properly`);
this.logger.error(e);
Expand All @@ -68,20 +62,21 @@ export class ReportingPlugin
// use data plugin for csv formats
setFieldFormats(plugins.data.fieldFormats);

const { logger } = this;
const reportingCore = this.getReportingCore();
const config = reportingCore.getConfig();
const { logger, reportingCore } = this;
const { elasticsearch } = reportingCore.getPluginSetupDeps();

// async background start
(async () => {
await this.reportingCore.pluginIsSetup();
const config = reportingCore.getConfig();

const browserDriverFactory = await initializeBrowserDriverFactory(config, logger);
reportingCore.setBrowserDriverFactory(browserDriverFactory);

const esqueue = await createQueueFactory(reportingCore, logger);
const enqueueJob = enqueueJobFactory(reportingCore, logger);
const esqueue = await createQueueFactory(reportingCore, logger); // starts polling for pending jobs
const enqueueJob = enqueueJobFactory(reportingCore, logger); // called from generation routes

reportingCore.pluginStart({
browserDriverFactory,
savedObjects: core.savedObjects,
uiSettings: core.uiSettings,
esqueue,
Expand All @@ -92,19 +87,11 @@ export class ReportingPlugin
runValidations(config, elasticsearch, browserDriverFactory, this.logger);

this.logger.debug('Start complete');
this.start$.next(true);
Copy link
Member Author

Choose a reason for hiding this comment

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

setup$ and start$ are no longer needed for createMockReportingCore

})().catch((e) => {
this.logger.error(`Error in Reporting start, reporting may not function properly`);
this.logger.error(e);
});

return {};
}

public getReportingCore() {
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 createMockReportingCore function has been refactored, and this is no longer needed.

if (!this.reportingCore) {
throw new Error('Setup is not ready');
}
return this.reportingCore;
}
}
16 changes: 7 additions & 9 deletions x-pack/plugins/reporting/server/routes/generation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,12 @@ import { HandlerFunction } from './types';

const esErrors = elasticsearchErrors as Record<string, any>;

export function registerJobGenerationRoutes(reporting: ReportingCore, logger: Logger) {
const getDownloadBaseUrl = (reporting: ReportingCore) => {
const config = reporting.getConfig();
const downloadBaseUrl =
config.kbnConfig.get('server', 'basePath') + `${API_BASE_URL}/jobs/download`;
return config.kbnConfig.get('server', 'basePath') + `${API_BASE_URL}/jobs/download`;
};

export function registerJobGenerationRoutes(reporting: ReportingCore, logger: Logger) {
/*
* Generates enqueued job details to use in responses
*/
Expand All @@ -42,6 +43,7 @@ export function registerJobGenerationRoutes(reporting: ReportingCore, logger: Lo

// return the queue's job information
const jobJson = job.toJSON();
const downloadBaseUrl = getDownloadBaseUrl(reporting);

return res.ok({
headers: {
Expand Down Expand Up @@ -86,10 +88,6 @@ export function registerJobGenerationRoutes(reporting: ReportingCore, logger: Lo
}

registerGenerateFromJobParams(reporting, handler, handleError);

// Register beta panel-action download-related API's
if (config.get('csv', 'enablePanelActionDownload')) {
registerGenerateCsvFromSavedObject(reporting, handler, handleError);
registerGenerateCsvFromSavedObjectImmediate(reporting, handleError, logger);
}
registerGenerateCsvFromSavedObject(reporting, handler, handleError); // FIXME: remove this https://github.com/elastic/kibana/issues/62986
registerGenerateCsvFromSavedObjectImmediate(reporting, handleError, logger);
}
Loading