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] Update more Server Types for TaskManager #74915

Merged
merged 13 commits into from
Aug 13, 2020

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Aug 12, 2020

Summary

Pulling changes out of #64853

Extends #65213

While working on removing ESQueue and using Task Manager, I found opportunistic ways to make the server types more strict, and allow some of the main modules such as ReportingStore to handle more corner cases. This PR takes out the opportunistic work out as a standalone PR.

List of changes:

  • Remove the ReportingCore.enqueueJob method since the route handler can just import enqueueJobFactory
  • Rename ESQueueCreateJobFn to CreateJobFn
  • Rename ESQueueWorkerExecuteFn to WorkerExecuteFn
  • Check the cancellation token in a loop in generate_csv
  • Define CreateJobBaseParams and CreateJobBaseParamsEncryptedFields to add stricter typing on job payloads (way less unknown in the system)
  • Remove ReportingStore.updateReport and add setReportClaimed, setReportCompleted, setReportFailed
  • Add some JSON formatting methods to the Report class
  • Make a copy of the ESQueue constants/statuses file for use outside of ESQueue

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@tsullivan tsullivan mentioned this pull request Aug 12, 2020
7 tasks
@tsullivan tsullivan changed the title Reporting/cleanup job param types [Reporting] Update more Server Types for TaskManager Aug 12, 2020
@tsullivan tsullivan added release_note:skip Skip the PR/issue when compiling release notes Team:Reporting Services v7.10.0 v8.0.0 labels Aug 12, 2020
@tsullivan tsullivan requested a review from joelgriffith August 13, 2020 16:04
@tsullivan tsullivan marked this pull request as ready for review August 13, 2020 16:04
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

@tsullivan
Copy link
Member Author

@elasticmachine merge upstream

@@ -113,6 +113,10 @@ export function createGenerateCsv(logger: LevelLogger) {
break;
}

if (cancellationToken.isCancelled()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an interesting one... wonder how we missed it for so long

const exportType = reporting.getExportTypesRegistry().getById(exportTypeId);

if (exportType == null) {
throw new Error(`Export type ${exportTypeId} does not exist in the registry!`);
}

// encrypt the headers in the payload
Copy link
Contributor

@joelgriffith joelgriffith Aug 13, 2020

Choose a reason for hiding this comment

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

Small nit: you can combine both 46 and 48 into a parallel await

const [
  scheduleTask,
  { store },
] = await Promise.all([
  exportType.scheduleTaskFnFactory(reporting, logger) as ScheduleTaskFnType,
  reporting.getPluginStartDeps()
]);

@@ -45,7 +45,7 @@ export const mapping = {
priority: { type: 'byte' },
timeout: { type: 'long' },
process_expiration: { type: 'date' },
created_by: { type: 'keyword' },
created_by: { type: 'keyword' }, // `null` if security is disabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Good callout

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think it used to be false but it sneakily changed through TS improvements. That shouldn't be a problem for mappings, I think.

Copy link
Contributor

@joelgriffith joelgriffith left a comment

Choose a reason for hiding this comment

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

Some small nits/perf improvements

@tsullivan
Copy link
Member Author

@joelgriffith I pushed up a commit per your feedback.

I also removed some more type-casting in the routes, because I couldn't resist thanks to the help from #74879

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@joelgriffith joelgriffith left a comment

Choose a reason for hiding this comment

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

Thanks for the context and cleanup of types! LGTM

jobParamsRison = jobParamsPayload;
} else {
const { jobParams: queryJobParams } = req.query as { jobParams: string };
const { jobParams: jobParamsPayload } = req.body;
Copy link
Contributor

Choose a reason for hiding this comment

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

yissssss that cleanup

@tsullivan tsullivan merged commit a8ed1f4 into elastic:master Aug 13, 2020
tsullivan added a commit to tsullivan/kibana that referenced this pull request Aug 13, 2020
* [Reporting] Update more Server Types for TaskManager

* remove some task manager references

* more strict

* more strict 2

* simplify

* fix test

* fix test

* routing validation unused types cleanup

* remove more casting in route handlers

* feedback changes

* original comment was fine

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
# Conflicts:
#	x-pack/plugins/reporting/server/export_types/printable_pdf/create_job/index.ts
#	x-pack/plugins/reporting/server/export_types/printable_pdf/types.d.ts
#	x-pack/plugins/reporting/server/lib/store/store.ts
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 14, 2020
* master: (23 commits)
  Adding API test for custom link transaction example (elastic#74238)
  [Uptime] Singular alert (elastic#74659)
  Revert "attempt excluding a codeowners directory" (elastic#75023)
  [Metrics UI] Remove TSVB dependency from Metrics Explorer APIs (elastic#74804)
  Remove degraded state from ES status service (elastic#75007)
  [Reporting/Functional] unskip pagination test (elastic#74973)
  [Resolver] Stale query string values are removed when resolver's component instance ID changes. (elastic#74979)
  Add public url to Workplace Search plugin (elastic#74991)
  [Reporting] Update more Server Types for TaskManager (elastic#74915)
  [I18n] verify select icu-message options are in english (elastic#74963)
  Make data.search.aggs available on the server. (elastic#74472)
  [Security Solution][Resolver] Graph Control Tests and Update Simulator Selectors (elastic#74680)
  attempt excluding a codeowners directory
  [ML] DF Analytics: allow failed job to be stopped by force via the UI (elastic#74710)
  Add kibana-core-ui-designers team (elastic#74970)
  [Metrics UI] Fix inventory footer misalignment (elastic#74707)
  Remove legacy optimizer (elastic#73154)
  Update design-specific GH code-owners (elastic#74877)
  skip test Reporting paginates content elastic#74922
  [Metrics UI] Add Jest tests for alert previews (elastic#74890)
  ...
tsullivan added a commit that referenced this pull request Aug 14, 2020
…75003)

* [Reporting] Update more Server Types for TaskManager (#74915)

* [Reporting] Update more Server Types for TaskManager

* remove some task manager references

* more strict

* more strict 2

* simplify

* fix test

* fix test

* routing validation unused types cleanup

* remove more casting in route handlers

* feedback changes

* original comment was fine

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
# Conflicts:
#	x-pack/plugins/reporting/server/export_types/printable_pdf/create_job/index.ts
#	x-pack/plugins/reporting/server/export_types/printable_pdf/types.d.ts
#	x-pack/plugins/reporting/server/lib/store/store.ts

* fix test

* fix test
@tsullivan tsullivan deleted the reporting/cleanup-job-param-types branch September 3, 2020 22:46
@sophiec20 sophiec20 added the (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead label Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants