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.

Sign up for GitHub

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

Jump to bottom

[Reporting] Capture CSV error codes #125812

Merged
merged 15 commits into from
Feb 17, 2022
Merged

[Reporting] Capture CSV error codes #125812

merged 15 commits into from
Feb 17, 2022

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Feb 16, 2022 •
edited
Loading

Summary

This PR partially addresses #125139

This contribution:

  • Establishes a pattern for getting error codes into a reporting job so they can be searched and aggregated
  • Adds custom error codes for auth expiration, queue timeouts and a catch-all unknown error

Screenshots

Screenshot 2022-02-16 at 17 05 50

Checklist

@jloleysens jloleysens added (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:skip Skip the PR/issue when compiling release notes Team:Reporting Services v8.2.0 labels Feb 16, 2022
@jloleysens jloleysens changed the title [Reporting] CSV error codes [Reporting] Capture CSV error codes Feb 16, 2022
@jloleysens jloleysens marked this pull request as ready for review February 17, 2022 09:24
@jloleysens jloleysens requested review from a team as code owners February 17, 2022 09:24
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServicesUx)

dokmic
dokmic approved these changes Feb 17, 2022
View reviewed changes
Copy link
Contributor

@dokmic dokmic left a comment

Choose a reason for hiding this comment

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

A nice feature that should be very helpful for debugging. The CI failures seem to be unrelated so that it looks good to me 👍

@jloleysens @dokmic
format per dokmic's suggestion
ba08dd3 
Co-authored-by: Michael Dokolin <[email protected]>
@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

ESLint disabled in files

id before after diff
reporting 1 2 +1

Total ESLint disabled count

id before after diff
reporting 9 10 +1

History

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

@jloleysens jloleysens merged commit c1d6322 into elastic:main Feb 17, 2022
@jloleysens jloleysens deleted the reporting/csv-error-codes branch February 17, 2022 15:23
tsullivan
tsullivan reviewed Feb 17, 2022
View reviewed changes
Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

Putting error_code under output would make more sense to me.

x-pack/plugins/reporting/common/types/index.ts
@@ -71,6 +71,7 @@ export interface ReportSource {
*/
jobtype: string; // refers to `ExportTypeDefinition.jobType`
created_by: string | false; // username or `false` if security is disabled. Used for ensuring users can only access the reports they've created.
error_code?: string;
Copy link
Member

Choose a reason for hiding this comment

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

@jloleysens would it be possible to move this field under output?

The organization of fields as it was, used top-level fields for things that may be defined when the job is created, and output had containment of the fields that may only be defined after report job execution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy with this suggestion, I'll move it in a follow up PR

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 21, 2022
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 125812 or prevent reminders by adding the backport:skip label.

@jloleysens jloleysens added backport:skip This commit does not require backporting and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Feb 21, 2022
@jloleysens jloleysens mentioned this pull request Feb 24, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsullivan tsullivan tsullivan left review comments

@dokmic dokmic dokmic approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:skip Skip the PR/issue when compiling release notes v8.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jloleysens @elasticmachine @kibana-ci @kibanamachine @tsullivan @dokmic