-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Changes from 10 commits
8eef541
26abb7d
8f381a7
99b16f6
ed011c0
1136503
7d75238
30d12fe
3142b18
a8ff18c
1e078fc
a7bae68
130bc7b
6e8d535
7d604bc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
@@ -33,7 +33,8 @@ export interface ReportingInternalSetup { | |
security?: SecurityPluginSetup; | ||
} | ||
|
||
interface ReportingInternalStart { | ||
export interface ReportingInternalStart { | ||
browserDriverFactory: HeadlessChromiumDriverFactory; | ||
enqueueJob: EnqueueJobFn; | ||
esqueue: ESQueueInstance; | ||
savedObjects: SavedObjectsServiceStart; | ||
|
@@ -43,33 +44,83 @@ interface ReportingInternalStart { | |
export class ReportingCore { | ||
private pluginSetupDeps?: ReportingInternalSetup; | ||
private pluginStartDeps?: ReportingInternalStart; | ||
private browserDriverFactory?: HeadlessChromiumDriverFactory; | ||
private readonly pluginSetup$ = new Rx.ReplaySubject<ReportingInternalSetup>(); | ||
private readonly pluginStart$ = new Rx.ReplaySubject<ReportingInternalStart>(); | ||
private readonly pluginSetup$ = new Rx.ReplaySubject<boolean>(); // observe async background setupDeps and config each are done | ||
private readonly pluginStart$ = new Rx.ReplaySubject<ReportingInternalStart>(); // observe async background startDeps | ||
private exportTypesRegistry = getExportTypesRegistry(); | ||
private config?: ReportingConfig; | ||
|
||
constructor(private config: ReportingConfig) {} | ||
constructor() {} | ||
|
||
public pluginSetup(reportingSetupDeps: ReportingInternalSetup) { | ||
this.pluginSetupDeps = reportingSetupDeps; | ||
this.pluginSetup$.next(reportingSetupDeps); | ||
/* | ||
* Register setupDeps | ||
*/ | ||
public pluginSetup(setupDeps: ReportingInternalSetup) { | ||
this.pluginSetup$.next(true); // trigger the observer | ||
this.pluginSetupDeps = setupDeps; // cache | ||
} | ||
|
||
public pluginStart(reportingStartDeps: ReportingInternalStart) { | ||
this.pluginStart$.next(reportingStartDeps); | ||
/* | ||
* Register startDeps | ||
*/ | ||
public pluginStart(startDeps: ReportingInternalStart) { | ||
this.pluginStart$.next(startDeps); // trigger the observer | ||
this.pluginStartDeps = startDeps; // cache | ||
} | ||
|
||
public pluginHasStarted(): Promise<boolean> { | ||
return this.pluginStart$.pipe(first(), mapTo(true)).toPromise(); | ||
/* | ||
* Blocks the caller until setup is done | ||
*/ | ||
public async pluginSetsUp(): 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 setBrowserDriverFactory(browserDriverFactory: HeadlessChromiumDriverFactory) { | ||
this.browserDriverFactory = browserDriverFactory; | ||
/* | ||
* Blocks the caller until start is done | ||
*/ | ||
public async pluginStartsUp(): Promise<boolean> { | ||
return await this.getPluginStartDeps().then(() => true); | ||
} | ||
|
||
/* | ||
* Synchronously checks if all async background setup and startup is completed | ||
*/ | ||
public pluginIsStarted() { | ||
return this.pluginSetupDeps != null && this.config != null && this.pluginStartDeps != null; | ||
} | ||
|
||
/* | ||
* Internal module dependencies | ||
* Allows config to be set in the background | ||
*/ | ||
public setConfig(config: ReportingConfig) { | ||
this.config = config; | ||
this.pluginSetup$.next(true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Full "plugin setup" now depends on 2 things: the |
||
} | ||
|
||
/* | ||
* Gives synchronous access to the config | ||
*/ | ||
public getConfig(): ReportingConfig { | ||
if (!this.config) { | ||
throw new Error('Config is not yet initialized'); | ||
} | ||
return this.config; | ||
} | ||
|
||
/* | ||
* Gives async access to the startDeps | ||
*/ | ||
private async getPluginStartDeps() { | ||
if (this.pluginStartDeps) { | ||
return this.pluginStartDeps; | ||
} | ||
|
||
return await this.pluginStart$.pipe(first()).toPromise(); | ||
} | ||
|
||
public getExportTypesRegistry() { | ||
return this.exportTypesRegistry; | ||
} | ||
|
@@ -92,37 +143,23 @@ export class ReportingCore { | |
.toPromise(); | ||
} | ||
|
||
public getConfig(): ReportingConfig { | ||
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); | ||
} | ||
|
||
/* | ||
* Gives synchronous access to the setupDeps | ||
*/ | ||
public getPluginSetupDeps() { | ||
if (!this.pluginSetupDeps) { | ||
throw new Error(`"pluginSetupDeps" dependencies haven't initialized yet`); | ||
} | ||
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; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -156,7 +156,7 @@ export async function generateCsvSearch( | |
}; | ||
|
||
const config = reporting.getConfig(); | ||
const elasticsearch = await reporting.getElasticsearchService(); | ||
const elasticsearch = reporting.getElasticsearchService(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ++ on always decreasing API surface area There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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>; | ||
|
@@ -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) { | ||
|
@@ -60,6 +56,8 @@ export function jobsQueryFactory( | |
}, | ||
}; | ||
|
||
const config = reportingCore.getConfig(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lazily resolving the config off of |
||
const index = config.get('index'); | ||
const query = { | ||
index: `${index}-*`, | ||
body: Object.assign(defaultBody[queryType] || {}, body), | ||
|
There was a problem hiding this comment.
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.