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 browser errors #127135

Merged
merged 8 commits into from
Mar 10, 2022
Merged

[Reporting] Capture browser errors #127135

merged 8 commits into from
Mar 10, 2022

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Mar 8, 2022 •
edited
Loading

Summary

Final piece of #125139

Adds special error classes to both screenshotting and reporting for capture browser related errors so that we can guide users better.

Screenshot 2022-03-07 at 16 06 26

Checklist

Delete any items that are not applicable to this PR.

@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 Mar 8, 2022
@jloleysens jloleysens requested a review from tsullivan March 8, 2022 11:09
@jloleysens jloleysens requested review from a team as code owners March 8, 2022 11:09
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

jloleysens and others added 2 commits March 8, 2022 12:12
@jloleysens
Merge branch 'main' of github.com:elastic/kibana into reporting/captu…
d4c9507 
…re-browser-errors

* 'main' of github.com:elastic/kibana: (46 commits)
  Fix copy and pasted renderer user_name test (elastic#126663)
  [Gauge] Vis editors gauge legacy percent mode. (elastic#126318)
  Remove all cases related code from timelines (elastic#127003)
  Hide Enterprise search panel when no nodes are present (elastic#127100)
  [Lens] Fixed flakiness on runtime fields' appearance on the list (elastic#126945)
  [Security Solution][Lists] - Add missing privileges callout to exception lists page (elastic#126874)
  [Security Solution][Lists] - Updates exception flyout edit error messages (elastic#126875)
  [Security Solution][Rules] - Remove rule selection for read only users (elastic#126827)
  Fix session cleanup test (elastic#126966)
  [ftr] implement support for accessing ES through CCS (elastic#126547)
  [type-summarizer] always use normalized paths, fix windows compat (elastic#127055)
  Revert "[ci] Configure hourly pipeline for a small spot instance trial (elastic#126824)"
  Revert "[CI] Expand spot instance trial a bit (elastic#126928)"
  [Alerting] Adding functional tests for alerting and actions telemetry (elastic#126528)
  [Telemetry] Check permissions when requesting telemetry (elastic#126238)
  Don't submit empty seed_urls or sitemap_urls when making a partial crawl request (elastic#126972)
  Remove License Requirement for Enterprise Search App Search Meta Engines (elastic#127046)
  [ML] Adding data recognizer module config cache (elastic#126338)
  skip flaky suite (elastic#126027)
  [Reporting] Improve error logging for rescheduled jobs (elastic#126737)
  ...

# Conflicts:
#	x-pack/plugins/reporting/server/lib/tasks/execute_report.ts
@jloleysens
update error tests snapshots
27bece0
tsullivan
tsullivan reviewed Mar 8, 2022
View reviewed changes
Copy link
Member

@tsullivan tsullivan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It seems unrelated to this, but I'm wondering why we can no longer have multiple warnings in the report job output.

Sorry about this mistaken comment. I needed to see the _formatOutput changes in a larger context. 👍

tsullivan
tsullivan approved these changes Mar 8, 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.

LGTM

@jloleysens
Merge branch 'main' of github.com:elastic/kibana into reporting/captu…
e2aebde 
…re-browser-errors

* 'main' of github.com:elastic/kibana: (46 commits)
  [Reporting] Capture Kibana stopped error (elastic#127017)
  add updatedAt to SimpleSavedObject (elastic#126359)
  Remove deprecated & unused `ElasticsearchServiceStart.legacy` (elastic#127050)
  remove opacity for fitting line series (elastic#127176)
  Remove deprecated & unused `HttpServiceSetup.auth` (elastic#127056)
  [Lens] Show underlying data editor navigation (elastic#125983)
  Bump dependencies (elastic#127238)
  Remove deprecated & unused `public-AsyncPlugin` (elastic#127048)
  Remove deprecated & unused `SavedObjectsImportFailure.title` (elastic#127043)
  skip flaky suite (elastic#123372)
  [kbn/generate] add basic package generator (elastic#127095)
  [build] Up compression quality (elastic#127064)
  Made fix to broken test. Deleted all existing pipelines before test starts. FLAKY: elastic#118593 (elastic#127102)
  Increase timeout for Jest integration tests (elastic#127220)
  skip failing test suite (elastic#126949)
  [DOCS] Adds note for data source performance impact (elastic#127184)
  [Security Solution] Adds CCS privileges warning enable switch in advanced settings (elastic#124459)
  [App Search] Move to tabbed single tabbed JSON flyout with upload and paste options and refactor cards (elastic#127162)
  Update dependency chromedriver to v99 (elastic#127079)
  [kbn/pm] add timings for more parts of bootstrap (elastic#127157)
  ...

# Conflicts:
#	x-pack/plugins/reporting/common/errors/index.ts
#	x-pack/plugins/reporting/server/lib/tasks/execute_report.ts
@jloleysens
Copy link
Contributor Author

Sorry about this mistaken comment. I needed to see the _formatOutput changes in a larger context. 👍

Yeah, I think what we need to do is not overload the warnings: string[] field with actual error messages. I guess the tricky part is that there could also be multiple error messages (although I can't think of such a case off the top of my head). Perhaps what we need is an error: string field on the report object. Arguably this is the function of content in the event of a failed report. Multiple directions to consider 🤪

Perhaps keeping warnings: string[] for ONLY warnings would be best?

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] Default CI Group #18 / apis Machine Learning filters create_filters should return 400 bad request if invalid filterId

Metrics [docs]

Unknown metric groups

ESLint disabled in files

id before after diff
screenshotting 0 1 +1

Total ESLint disabled count

id before after diff
screenshotting 2 3 +1

History

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

@jloleysens jloleysens merged commit 714f3b2 into elastic:main Mar 10, 2022
@jloleysens jloleysens added the backport:skip This commit does not require backporting label Mar 10, 2022
jloleysens added a commit to jloleysens/kibana that referenced this pull request Mar 10, 2022
@jloleysens
Merge branch 'main' of github.com:elastic/kibana into screenshotting/…
6cb4725 
…move-pdf-generation-to-screenshotting

* 'main' of github.com:elastic/kibana: (62 commits)
  [Lens] Drop partial buckets option (elastic#127153)
  chore(NA): remove unused translation xpack.ml.management.jobsSpacesList.objectNoun from fr-FR (elastic#127457)
  Add data to user details page (elastic#127019)
  [Fleet] Make upload and registry package info consistent (elastic#126915)
  [Reporting] Capture browser errors (elastic#127135)
  Initial readme commit with some stub articles (elastic#127420)
  skip flaky suite (elastic#121482)
  skip flaky suite (elastic#127416)
  Tests to ensure Kibana is handling multi-space import of saved objects correctly (elastic#127229)
  [Aggs] remove toAngularJson (elastic#127267)
  [i18n] Integrate 8.2.0 Translations (elastic#127309)
  [Security Solution] [Endpoint] Creates generic policy tab artifact component to be used for all of our artifacts (elastic#126685)
  [Kibana React] Fix Page Template `solutionNav` propagation (elastic#127140)
  [Cases] Export getRelatedCases API from cases client (elastic#127065)
  [Cloud Posture]add support for sorting benchmark page (elastic#126983)
  [User experience] Fix filters for the app (elastic#127295)
  [Fleet] Fix timeserie dimension mapping (elastic#127328)
  [data view mgmt] fix data view name wrap (elastic#127319)
  [kbn/optimizer] extract string diffing logic (elastic#127394)
  [ResponseOps] Add pagination and sorting to the alerts search strategy (elastic#126813)
  ...

# Conflicts:
#	x-pack/plugins/screenshotting/common/errors.ts
#	x-pack/plugins/screenshotting/common/index.ts
#	x-pack/plugins/screenshotting/server/screenshots/observable.ts
@tsullivan tsullivan mentioned this pull request Apr 12, 2022
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsullivan tsullivan tsullivan 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.
4 participants
@jloleysens @elasticmachine @kibana-ci @tsullivan