Export Type Registry for Reporting [POC/exploration] #155081
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…le' into serverless-image-reporting-disable
…le' into serverless-image-reporting-disable
…le' into serverless-image-reporting-disable
…le' into serverless-image-reporting-disable
…le' into serverless-image-reporting-disable
…le' into serverless-image-reporting-disable
tsullivan
reviewed
May 8, 2023
Comment on lines
9
to
12
| "configPath": [ | ||
| "xpack", | ||
| "reportingExportTypes" | ||
| ], |
There was a problem hiding this comment.
Looks like this causes the server to crash, since there are no plugins that export a config schema with this path.
For now, we can re-use the configPath from reporting:
Suggested change
| "configPath": [ | |
| "xpack", | |
| "reportingExportTypes" | |
| ], | |
| "configPath": [ | |
| "xpack", | |
| "reporting" | |
| ], |
tsullivan
reviewed
May 8, 2023
Comment on lines
50
to
82
| /** | ||
| * Export types to the central reporting plugin | ||
| */ | ||
| type CreateFnType = CreateJobFn<any, any>; // can not specify params types because different type of params are not assignable to each other | ||
| type RunFnType = any; // can not specify because ImmediateExecuteFn is not assignable to RunTaskFn | ||
| const getTypeFns: Array<() => ExportTypeDefinition<CreateFnType | null, RunFnType>> = [ | ||
| getTypeCsv, | ||
| getTypeCsvFromSavedObject, | ||
| getTypeCsvFromSavedObjectImmediate, | ||
| getTypePng, | ||
| getTypePngV2, | ||
| getTypePrintablePdf, | ||
| getTypePrintablePdfV2, | ||
| ]; | ||
| getTypeFns.forEach((getType) => { | ||
| if ( | ||
| getType === getTypeCsv || | ||
| getType === getTypeCsvFromSavedObject || | ||
| getType === getTypeCsvFromSavedObjectImmediate | ||
| ) { | ||
| return ( | ||
| // setFieldFormats(fieldFormats), | ||
| reporting.registerExportType( | ||
| getType() as unknown as ExportTypeDefinition<CreateJobFn<any, any>, RunTaskFn<any>> | ||
| ), | ||
| setFieldFormats(fieldFormats) | ||
| ); | ||
| } else { | ||
| return reporting.registerExportType( | ||
| getType() as unknown as ExportTypeDefinition<CreateJobFn<any, any>, RunTaskFn<any>> | ||
| ); | ||
| } | ||
| }); |
There was a problem hiding this comment.
I think we can simplify this section, and make it more readable without the .forEach:
/**
* Export types to the central reporting plugin
*/
reporting.registerExportType(getTypeCsv());
reporting.registerExportType(getTypeCsvFromSavedObject());
// @ts-ignore
reporting.registerExportType(getTypeCsvFromSavedObjectImmediate());
reporting.registerExportType(getTypePng());
reporting.registerExportType(getTypePngV2());
reporting.registerExportType(getTypePrintablePdf());
reporting.registerExportType(getTypePrintablePdfV2());
I put the ts-ignore over the getTypeCsvFromSavedObjectImmediate because the definition for that export type is a strange one. But I think it's ok to ignore the TS error for now, since we want to remove that export type: #145937
tsullivan
reviewed
May 8, 2023
| } | ||
|
|
||
| // do nothing | ||
| public start({}, {}) {} |
There was a problem hiding this comment.
Within this lifecycle method, we want to make the setFieldFormats call:
Suggested change
| public start({}, {}) {} | |
| public start(_core: CoreStart, plugins: ExportTypesPluginStartDependencies) { | |
| setFieldFormats(plugins.fieldFormats); | |
| } |
And ExportTypesPluginStartDependencies will just be:
export interface ExportTypesPluginStartDependencies {
fieldFormats: FieldFormatsStart;
}
tsullivan
reviewed
May 8, 2023
Comment on lines
+89
to
+105
| public getPluginSetupDeps() { | ||
| if (!this.pluginSetupDeps) { | ||
| throw new Error(`"pluginSetupDeps" dependencies haven't initialized yet`); | ||
| } | ||
| return this.pluginSetupDeps; | ||
| } | ||
|
|
||
| /* | ||
| * Gives async access to the startDeps | ||
| */ | ||
| public async getPluginStartDeps() { | ||
| if (this.pluginStartDeps) { | ||
| return this.pluginStartDeps; | ||
| } | ||
|
|
||
| return await Rx.firstValueFrom(this.pluginStart$); | ||
| } |
There was a problem hiding this comment.
Do we just need these 2 methods from this class?
tsullivan
reviewed
May 8, 2023
| - PNGExportTypeDefinition | ||
| - PDFExportTypeDefinition | ||
|
|
||
| The `ReportingPlugin` calls the `ReportingExportTypesPlugin` and creates an instance of the `ExportTypeRegistry`. These export types use the `ReportingPlugin`'s registerExportType() within the `ExportTypeRegistry`. The `ReportingPlugin` uses the getExportType() method to use the registered export types for exporting. |
There was a problem hiding this comment.
Suggested change
| The `ReportingPlugin` calls the `ReportingExportTypesPlugin` and creates an instance of the `ExportTypeRegistry`. These export types use the `ReportingPlugin`'s registerExportType() within the `ExportTypeRegistry`. The `ReportingPlugin` uses the getExportType() method to use the registered export types for exporting. | |
| The `ReportingPlugin` creates an instance of the `ExportTypeRegistry` and exposes its `registerExportType()` to other plugins, which `ReportingExportTypesPlugin` uses to register all of the export types. The `ReportingPlugin` uses the getExportType() method to use the registered export types for exporting. |
tsullivan
reviewed
May 9, 2023
| @@ -31,6 +31,7 @@ export function isConfigPath(value: unknown): value is ConfigPath { | |||
| * @internal | |||
| */ | |||
| export interface Config { | |||
| kbnConfig: any; | |||
There was a problem hiding this comment.
I see the confusion here. Reporting has a strange object that gets passed around from a class called ReportingConfig. The reasons we do this go way back, but I believe that passing config around to the parts that need it might be simpler now.
Given the impact of this, I believe we should add a granular step #152845: Refactor ReportingConfig to just use a simple type.
@rshen91 Would you mind if we put this PR on pause while we look at that?
There was a problem hiding this comment.
I think this simplifies the base code quite a lot and hopefully makes things in this PR easier: #157118.
💔 Build FailedFailed CI Steps
Test Failures
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @rshen91 |
tsullivan
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
#152845
This PR will lend itself to closing (but doesn't close this) - #154675
The different export types from
ReportingExportTypesplugin will register themselves withinReportingPlugin's export types registry and the registry will have the allowed export types for serverless.Checklist
Delete any items that are not applicable to this PR.