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

Always use summary events for monitor info #82376

Merged

Conversation

andrewvc
Copy link
Contributor

@andrewvc andrewvc commented Nov 2, 2020

Fixes #81942

With synthetics we no longer are guaranteed to have the URL or other monitor info fields in every single event. This PR patches the monitor status API to only query summary fields for this info.

As an added benefit, this is a bit more consistent as well because we tend to only show data from completed, not partial checks, in most places.

This also removes the status check parameter from this API, which otherwise would need to be refactored. Checking the codebase it appears unused.

Summary

Summarize your PR. If it involves visual changes include a screenshot or gif.

For maintainers

Fixes elastic#81942

With synthetics we no longer are guaranteed to have the URL or other
monitor info fields in every single event. This PR patches the monitor
status API to only query summary fields for this info.

As an added benefit, this is a bit more consistent as well because we
tend to only show data from completed, not partial checks, in most
places.

This also removes the status check parameter from this API, which
otherwise would need to be refactored. Checking the codebase it appears
unused.
@andrewvc andrewvc added Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability release_note:skip Skip the PR/issue when compiling release notes labels Nov 2, 2020
@andrewvc andrewvc requested a review from a team as a code owner November 2, 2020 21:09
@andrewvc andrewvc self-assigned this Nov 2, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@andrewvc andrewvc added bug Fixes for quality problems that affect the customer experience v7.10.0 labels Nov 2, 2020
Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

LGTM, make sense to me !!

@@ -46,7 +47,6 @@ export const getLatestMonitor: UMElasticsearchQueryFn<GetLatestMonitorParams, Pi
},
},
},
...(status ? [{ term: { 'monitor.status': status } }] : []),
Copy link
Contributor

Choose a reason for hiding this comment

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

You will also need to remove status from the function receiving params

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@andrewvc andrewvc merged commit e94db63 into elastic:master Nov 3, 2020
@andrewvc andrewvc deleted the get-monitor-status-from-summary-only branch November 3, 2020 02:10
andrewvc added a commit to andrewvc/kibana that referenced this pull request Nov 3, 2020
* Always use summary events for monitor info

Fixes elastic#81942

With synthetics we no longer are guaranteed to have the URL or other
monitor info fields in every single event. This PR patches the monitor
status API to only query summary fields for this info.

As an added benefit, this is a bit more consistent as well because we
tend to only show data from completed, not partial checks, in most
places.

This also removes the status check parameter from this API, which
otherwise would need to be refactored. Checking the codebase it appears
unused.

* Remove unneeded params
andrewvc added a commit to andrewvc/kibana that referenced this pull request Nov 3, 2020
* Always use summary events for monitor info

Fixes elastic#81942

With synthetics we no longer are guaranteed to have the URL or other
monitor info fields in every single event. This PR patches the monitor
status API to only query summary fields for this info.

As an added benefit, this is a bit more consistent as well because we
tend to only show data from completed, not partial checks, in most
places.

This also removes the status check parameter from this API, which
otherwise would need to be refactored. Checking the codebase it appears
unused.

* Remove unneeded params
andrewvc added a commit that referenced this pull request Nov 3, 2020
* Always use summary events for monitor info

Fixes #81942

With synthetics we no longer are guaranteed to have the URL or other
monitor info fields in every single event. This PR patches the monitor
status API to only query summary fields for this info.

As an added benefit, this is a bit more consistent as well because we
tend to only show data from completed, not partial checks, in most
places.

This also removes the status check parameter from this API, which
otherwise would need to be refactored. Checking the codebase it appears
unused.

* Remove unneeded params
andrewvc added a commit that referenced this pull request Nov 3, 2020
* Always use summary events for monitor info

Fixes #81942

With synthetics we no longer are guaranteed to have the URL or other
monitor info fields in every single event. This PR patches the monitor
status API to only query summary fields for this info.

As an added benefit, this is a bit more consistent as well because we
tend to only show data from completed, not partial checks, in most
places.

This also removes the status check parameter from this API, which
otherwise would need to be refactored. Checking the codebase it appears
unused.

* Remove unneeded params
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug][Uptime] URL field sometimes blank for Synthetic monitor
4 participants