-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[Reporting/New Platform] Provide async access to server-side dependencies #56824
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/New Platform] Provide async access to server-side dependencies #56824
Conversation
fbc9163 to
17d04f1
Compare
|
Pinging @elastic/kibana-reporting-services (Team:Reporting Services) |
💔 Build Failed
Test FailuresKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/advanced_settings/feature_controls/advanced_settings_security·ts.Advanced Settings security feature controls "before all" hookStandard OutStack TraceKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/advanced_settings/feature_controls/advanced_settings_security·ts.Advanced Settings security feature controls "after all" hook in "security feature controls"Standard OutStack TraceKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/dashboard_mode/dashboard_view_mode·js.dashboard mode Dashboard View Mode "before all" hook: initialize testsStandard OutStack Traceand 51 more failures, only showing the first 3. History
To update your PR or re-run it, just comment with: |
5e89c52 to
073cbcf
Compare
9d57ada to
807be16
Compare
807be16 to
1a9e839
Compare
1a9e839 to
25dc761
Compare
c391413 to
dd0fd40
Compare
|
|
||
| const savedObjects = server.savedObjects; | ||
| const savedObjectsClient = savedObjects.getScopedSavedObjectsClient(fakeRequest); | ||
| const uiSettings = server.uiSettingsServiceFactory({ savedObjectsClient }); |
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.
Removed legacy dependency here
| ); | ||
| const uiConfig = server.uiSettingsServiceFactory({ | ||
| savedObjectsClient, | ||
| }); |
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.
Removed legacy dependency here
| reporting: reportingPlugin, | ||
| }, | ||
| savedObjects: server.savedObjects, | ||
| uiSettingsServiceFactory: server.uiSettingsServiceFactory, |
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.
Removed legacy dependencies here
| ): Promise<CsvResultFromSearch> { | ||
| const { savedObjects, uiSettingsServiceFactory } = server; | ||
| const savedObjectsClient = savedObjects.getScopedSavedObjectsClient( | ||
| const savedObjectsClient = await reporting.getSavedObjectsClient( |
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.
Removed legacy dependency here and from line 76
| { browserDriverFactory }: { browserDriverFactory: HeadlessChromiumDriverFactory } | ||
| parentLogger: Logger | ||
| ) { | ||
| const browserDriverFactory = await reporting.getBrowserDriverFactory(); |
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.
We're also using other accessor methods to get internal dependencies that are available after setup, like this getBrowserDriverFactory and even getEsqueue
| elasticsearch: ElasticsearchServiceSetup, | ||
| logger: Logger, | ||
| { exportTypesRegistry, browserDriverFactory }: CreateQueueFactoryOpts | ||
| ): Esqueue { |
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.
Since more accessors exist on the reporting object, I have simplified signatures like this because the extra opts no longer need to be passed separately
pgayvallet
left a comment
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.
The approach chosen to be able to pass dependencies only available during start to the route handlers that must be defined during setup definitely makes sense, and follows the suggested approach.
As explained in the comments:
- I would still avoid passing the whole and concrete
ReportingPlugininstance alongs the whole chain, and use a specialized type or at least interface for that - The plugin's
start(andsetupalso it seems) method is currently returning an internal contract instead of a public (or empty) one. From what I see, these returns even seems unused as the calls inx-pack/legacy/plugins/reporting/server/legacy.tsare ignoring them, so I would remove that
| logger.error(err); | ||
| logger.warning( |
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.
NIT: logging both an error and a warning?
| xpackMainPlugin.info.feature(PLUGIN_ID).registerLicenseCheckResultsGenerator(checkLicense); | ||
| }); | ||
| // Reporting routes | ||
| registerRoutes(this, __LEGACY, plugins, this.logger); |
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.
For isolation and testability sake, I would really avoid using the concrete ReportingPlugin type for the registerRoutes (and every nested calls) signature, and use a specialized interface instead. Actually passing the plugin instance as the implementation is fine I think, but I would create a real fascade interface containing only the internally used methods, to at least excludes start and setup from the contract, something like Pick<ReportingPlugin, 'getEsqueue' | ...>
Another slight inprovement (imho) from this initial proposal would be to extract this contract you put in the ReportingPlugin class to another class and use it instead
Something like
const internalContract = new ReportingInternals(this.pluginStartDeps, this.exportTypesRegistry, [probably others]);
registerRoutes(internalContract, __LEGACY, plugins, this.logger);|
|
||
| // Reporting routes | ||
| registerRoutes(__LEGACY, plugins, exportTypesRegistry, browserDriverFactory, logger); | ||
| return this.pluginStartDeps; |
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.
This is supposed to be the plugin's public contract. You are currently returning an internal contract, which doesn't really make any sense of any plugins that may want to consume your API (even if I get that reporting is more of a 'end of chain' plugin, this still doesn't respect the core conventions)
this.pluginStartDeps = {
savedObjects: core.savedObjects,
uiSettings: core.uiSettings,
esqueue,
enqueueJob,
};My question is: I see you are using that internally (this.pluginStart$.next(this.pluginStartDeps);), but do you use anywhere this contract from the actual plugin's start return?
- if no, I would stick to returning an empty contract (
{}) - If you do need to access this from outside of the shim for any reason, I would wrap it inside an
__internalproperty to make it clear that this a temporary measure during migration
This follows my previous comment, I think your pluginStartDeps should be more named such as reportingInternalStartDeps or something like that, as it is supposed to be only consumed from inside your plugin.
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.
It makes senses to use a new types for Internal* instead of hooking into the types that were conveniently there :). Since I chose to use a type conveniently in scope, that forced me to return it. I get the point that it should only public expose internals as-needed by outside consumers
if no, I would stick to returning an empty contract ({})
👍
| JobDocPayloadDiscoverCsv | ||
| >> = function executeJobFactoryFn( | ||
| >> = async function executeJobFactoryFn( | ||
| reporting: ReportingPlugin, | ||
| server: ServerFacade, | ||
| elasticsearch: ElasticsearchServiceSetup, | ||
| parentLogger: Logger |
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.
Minor: now that you introduced this ReportingPlugin internal that currently contains SO accessors such as reporting.getSavedObjectsClient(fakeRequest), it would probably makes sense to move the elasticsearch accessors inside it too instead of passing it as an additional parameter, for consistency.
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.
the thought occurred to me as well, so 👍
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.
I'd like to take this on in a future PR. It would make sense to get this in before working on the migration of our "config" usage on the server. Config can also follow the pattern of using an accessor on the reporting object.
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.
fine with me
pgayvallet
left a comment
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.
LGTM for platform changes
|
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
…cies (elastic#56824) * [Reporting/New Platform] Provide async access to server-side * consistent name for reportingPlugin * Prettier changes * simplify reporting usage collector setup * add more tests * extract internals access to separate core class * fix tests * fix imports for jest and build Co-authored-by: Elastic Machine <[email protected]>
…pendencies (#56824) (#57727) * [Reporting/New Platform] Provide async access to server-side dependencies (#56824) * [Reporting/New Platform] Provide async access to server-side * consistent name for reportingPlugin * Prettier changes * simplify reporting usage collector setup * add more tests * extract internals access to separate core class * fix tests * fix imports for jest and build Co-authored-by: Elastic Machine <[email protected]> * fix lint * fix ts import Co-authored-by: Elastic Machine <[email protected]>
Summary
Related #53898
Migrates the Reporting plugin server-side to access these dependences outside of the context of a request:
savedObjectsClientuiSettingsServiceSince these dependences are available in the
startmethod of the new ReportingPlugin class, I have moved somesetupcode tostart:esqueueenqueueJobFnPrevious
initfunctionality is still run in thesetupmethod, such as validating the configuration (and setting default encryption key) and setting the routes.In this PR, the ReportingPlugin class provides async getters for all of these dependencies. The
reportingobject is now passed to route registration and is now part of the export type definitions. This is a new facade utilized by a large part of the Reporting server-side code, which amounts to a lot of lines of code change in this PR.Checklist
Delete any items that are not applicable to this PR.
[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support[ ] Documentation was added for features that require explanation or tutorials[ ] This was checked for keyboard-only and screenreader accessibility[ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser[ ] This was checked for cross-browser compatibility, including a check against IE11For maintainers