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

[Uptime] Monitor status alert use url as instance #81736

Merged
merged 16 commits into from
Nov 16, 2020
Merged

[Uptime] Monitor status alert use url as instance #81736

merged 16 commits into from
Nov 16, 2020

Conversation

shahzad31
Copy link
Contributor

@shahzad31 shahzad31 commented Oct 27, 2020 •
edited
Loading

Summary

Fixes: #80932

For uptime monitor status alert, using url as instance id, so that user can have better understanding which alert instance they need to mute.

Also update table layout in alerting UI to make it more user friendly.

After:
image

Before:
image

@shahzad31 shahzad31 marked this pull request as ready for review October 27, 2020 09:42
@shahzad31 shahzad31 requested review from a team as code owners October 27, 2020 09:42
@shahzad31 shahzad31 changed the title Use url as instance [UPTIME] Monitor status alert use url as instance Oct 27, 2020
@shahzad31 shahzad31 changed the title [UPTIME] Monitor status alert use url as instance [Uptime] Monitor status alert use url as instance Oct 27, 2020
@shahzad31 shahzad31 mentioned this pull request Oct 27, 2020
Closed
@botelastic botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Oct 27, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@shahzad31
Copy link
Contributor Author

@elasticmachine merge upstream

mdefazio
mdefazio approved these changes Nov 2, 2020
View reviewed changes
Copy link
Contributor

@mdefazio mdefazio left a comment

Choose a reason for hiding this comment

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

As far as how the table is used, I think this is good. I agree that the truncation / flexibility should occur on the instance/url field, even though there are times that the unique part will be cut off.

@shahzad31
Copy link
Contributor Author

@elasticmachine merge upstream

andrewvc
andrewvc requested changes Nov 5, 2020
View reviewed changes
Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

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

Mostly looks good, but after thinking about it more monitor.name makes the most sense to prioritize in the display

x-pack/plugins/uptime/server/lib/alerts/status_check.ts Outdated Show resolved Hide resolved
x-pack/plugins/uptime/server/lib/alerts/status_check.ts Outdated Show resolved Hide resolved
andrewvc
andrewvc approved these changes Nov 5, 2020
View reviewed changes
Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

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

LGTM

gmmorris
gmmorris approved these changes Nov 6, 2020
View reviewed changes
Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

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

LGTM, seems to be behaving as expected :)
My only concern is about hard coded px values and I've pinged Design to get their thoughts, but if they're good with it then ✅

@shahzad31
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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
triggersActionsUi 1.4MB 1.4MB +315.0B

History

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

@shahzad31 shahzad31 merged commit daa7cc9 into elastic:master Nov 16, 2020
@shahzad31 shahzad31 deleted the use-url-as-instance-id branch November 16, 2020 10:54
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 16, 2020
@gmmorris
Merge branch 'master' into alerts/action-groups-as-conditions
c54458f 
* master:
  Migrate `/translations` route to core (elastic#83280)
  [APM] Ensure APM jest script can run (elastic#83398)
  [Uptime] Monitor status alert use url as instance (elastic#81736)
  [ML] Add basic license test run details to ML+Transform READMEs (elastic#83259)
  TSVB doesn't communicate it's index-patterns to dashboard (elastic#82964)
  [Alerting UI] Added ability to assign alert actions to resolved action group in UI (elastic#83139)
  Skips Vega test
  skip flaky suite (elastic#79967)
  [bundle optimization] Update to semver 7.x to get tree-shaking (elastic#83020)
  Added ability to fire actions when an alert instance is resolved (elastic#82799)
  [ML] Adds functional tests for the index data visualizer card contents (elastic#83174)
@shahzad31 shahzad31 mentioned this pull request Nov 16, 2020
Merged
shahzad31 added a commit to shahzad31/kibana that referenced this pull request Nov 16, 2020
@shahzad31 @kibanamachine
[Uptime] Monitor status alert use url as instance (elastic#81736)
2c92367 
Co-authored-by: Kibana Machine <[email protected]>
phillipb added a commit to phillipb/kibana that referenced this pull request Nov 16, 2020
@phillipb
Merge branch 'add-logs-to-node-details' of github.com:phillipb/kibana…
b58ece4 
… into add-logs-to-node-details

* 'add-logs-to-node-details' of github.com:phillipb/kibana:
  fix tall vislib charts in visualize (elastic#83340)
  [Lens] Avoid unnecessary data fetching on dimension flyout open (elastic#82957)
  [Security Solution][Case] Change case connector minimum required license to basic (elastic#83401)
  fix logstash central pipeline management test  (elastic#83281)
  [Search] Send to background UI (elastic#81793)
  Migrate `/translations` route to core (elastic#83280)
  [APM] Ensure APM jest script can run (elastic#83398)
  [Uptime] Monitor status alert use url as instance (elastic#81736)
  [ML] Add basic license test run details to ML+Transform READMEs (elastic#83259)
  TSVB doesn't communicate it's index-patterns to dashboard (elastic#82964)
  [Alerting UI] Added ability to assign alert actions to resolved action group in UI (elastic#83139)
  Skips Vega test
  skip flaky suite (elastic#79967)
  [bundle optimization] Update to semver 7.x to get tree-shaking (elastic#83020)
  Added ability to fire actions when an alert instance is resolved (elastic#82799)
shahzad31 added a commit that referenced this pull request Nov 16, 2020
@shahzad31 @kibanamachine
[7.x] [Uptime] Monitor status alert use url as instance (#81736) (#83412
3d808fb 
)

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
@gmmorris gmmorris mentioned this pull request Nov 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryankeairns ryankeairns ryankeairns left review comments

@pmuellr pmuellr pmuellr left review comments

@mdefazio mdefazio mdefazio approved these changes

@gmmorris gmmorris gmmorris approved these changes

@andrewvc andrewvc andrewvc approved these changes

Assignees
No one assigned
Labels
release_note:fix Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.11.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow muting of Uptime created alerts based on url/endpoint.
8 participants
@shahzad31 @elasticmachine @kibanamachine @pmuellr @andrewvc @gmmorris @ryankeairns @mdefazio