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

[Synthetics] Private location better error handling #152695

Merged
merged 18 commits into from
May 5, 2023

Conversation

shahzad31
Copy link
Contributor

@shahzad31 shahzad31 commented Mar 6, 2023

Fixes #147728

Private location better error handling, error handling has been done to make sure, if somehow creating package policy fails, it properly reflects in the UI and also for project monitors

Testing Scope

Since these PR touches lot of public/private location code paths, following test should be performed

UI Monitors

  • Creating monitor in on self hosted and on cloud
  • Editing monitor with multiple locations in self hosted and on
  • Test that if creating/editing integration fails, it shows error in the UI

Project monitors

  • Add/edit/delete single project monitors
  • Add/edit/delete multiple project monitors
  • Add multiple monitors but one of the should fail with some error
  • Edit multiple monitors but one of them should fail with error

@shahzad31 shahzad31 force-pushed the better-error-handling branch from a5252ba to 6ccf699 Compare May 2, 2023 16:22
@dominiqueclarke dominiqueclarke self-requested a review May 3, 2023 00:40
Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

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

An internal server error happens during the editing flow

<img width="798" alt="Screen Shot 2023-05-02 at 9 08 28 PM" src="https://user-images.githubusercontent.com/11356435/235816127-92f79d9e-e77c-4329-a7e7-4bf0bca79652.png">

Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

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

After removing the routeContext issue with a commit I pushed, I'm seeing this issue on the edit monitor flow.

proc [kibana] [2023-05-02T21:32:15.814-04:00][ERROR][http] ResponseError: [1:360] [bool] failed to parse field [filter]: x_content_parse_exception
 proc [kibana]  Caused by:
 proc [kibana]          x_content_parse_exception: [1:360] [bool] failed to parse field [should]
 proc [kibana]  Root causes:
 proc [kibana]          parsing_exception: [terms] query does not support [namespaces]
 proc [kibana]     at KibanaTransport.request (/Users/dominiqueclarke/dev/kibana/node_modules/@elastic/transport/src/Transport.ts:535:17)
 proc [kibana]     at runMicrotasks (<anonymous>)
 proc [kibana]     at processTicksAndRejections (node:internal/process/task_queues:96:5)

@shahzad31 shahzad31 force-pushed the better-error-handling branch from 16e2e90 to e5da186 Compare May 3, 2023 07:38
@shahzad31 shahzad31 marked this pull request as ready for review May 3, 2023 12:14
@shahzad31 shahzad31 requested review from a team as code owners May 3, 2023 12:14
@shahzad31 shahzad31 added release_note:skip Skip the PR/issue when compiling release notes v8.8.0 v8.9.0 labels May 3, 2023
@dominiqueclarke dominiqueclarke self-requested a review May 3, 2023 13:37
@botelastic botelastic bot added Team:Fleet Team label for Observability Data Collection Fleet team Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels May 3, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

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

An error originating from public locations, for example addConfigs in the synthetics service, does not delete the integration policy, which was already created since public and private locations happen in parallel.

This creates an orphaned integration policy.

We should ensure that the integration policy is deleted if we encounter issues in public locations.

@dominiqueclarke dominiqueclarke self-requested a review May 4, 2023 13:23
Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

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

Synthetics LGTM. The previous issue has been resolved.

export type DecryptedSyntheticsMonitorSavedObject = SimpleSavedObject<SyntheticsMonitor> & {
updated_at: string;
};

export interface SyntheticsServiceAllowed {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these not in use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah they aren't.

@nchaulet nchaulet self-requested a review May 4, 2023 18:56
Copy link
Member

@nchaulet nchaulet left a comment

Choose a reason for hiding this comment

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

Fleet changes LGTM, it seems synthetics are the only using those bulk methods so it's probably okay to change those method behaviours

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #45 / expressions explorer emits an action and navigates

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
synthetics 1.2MB 1.2MB +63.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 48 47 -1
securitySolution 398 401 +3
total +4

References to deprecated APIs

id before after diff
synthetics 96 95 -1

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 58 57 -1
securitySolution 478 481 +3
total +4

History

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

@shahzad31 shahzad31 merged commit 1aba7df into elastic:main May 5, 2023
@shahzad31 shahzad31 deleted the better-error-handling branch May 5, 2023 09:32
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request May 5, 2023
Co-authored-by: Dominique Clarke <dominique.clarke@elastic.co>
(cherry picked from commit 1aba7df)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.8

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request May 8, 2023
…156807)

# Backport

This will backport the following commits from `main` to `8.8`:
- [[Synthetics] Private location better error handling
(#152695)](#152695)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT
[{"author":{"name":"Shahzad","email":"shahzad31comp@gmail.com"},"sourceCommit":{"committedDate":"2023-05-05T09:31:59Z","message":"[Synthetics]
Private location better error handling (#152695)\n\nCo-authored-by:
Dominique Clarke
<dominique.clarke@elastic.co>","sha":"1aba7df3363d25a24b35fd0f716d5f89c76c41b8","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:uptime","release_note:skip","Team:Fleet","v8.8.0","v8.9.0"],"number":152695,"url":"https://github.com/elastic/kibana/pull/152695","mergeCommit":{"message":"[Synthetics]
Private location better error handling (#152695)\n\nCo-authored-by:
Dominique Clarke
<dominique.clarke@elastic.co>","sha":"1aba7df3363d25a24b35fd0f716d5f89c76c41b8"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"8.8","label":"v8.8.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/152695","number":152695,"mergeCommit":{"message":"[Synthetics]
Private location better error handling (#152695)\n\nCo-authored-by:
Dominique Clarke
<dominique.clarke@elastic.co>","sha":"1aba7df3363d25a24b35fd0f716d5f89c76c41b8"}}]}]
BACKPORT-->

Co-authored-by: Shahzad <shahzad31comp@gmail.com>
Co-authored-by: Dominique Clarke <dominique.clarke@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.8.0 v8.9.0
Projects
None yet
6 participants