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

[Uptime] Remove overview page scripted metric query for monitor list #68096

Closed
andrewvc opened this issue Jun 3, 2020 · 1 comment · Fixed by #69229
Closed

[Uptime] Remove overview page scripted metric query for monitor list #68096

andrewvc opened this issue Jun 3, 2020 · 1 comment · Fixed by #69229
Labels
Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability technical debt Improvement of the software architecture and operational architecture

Comments

@andrewvc
Copy link
Contributor

andrewvc commented Jun 3, 2020

This is unnecessary tech debt. We can replace it with a very simple query for the check group documents.

@andrewvc andrewvc added technical debt Improvement of the software architecture and operational architecture Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Jun 3, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

andrewvc added a commit that referenced this issue Jul 9, 2020
Resolves #68096 , removing unnecessary scripted metric query from overview page and unifying the Check and Ping types.

This simplifies the types in a number of ways, and reduces the total quantity of code to execute the queries for the overview page. It also fixes the Tls and related types which were inconsistent and presented a problem here since they are used by this JS. There are now three stages where before there were four:

    Find potential matches: where we determine which monitor IDs are eligible for the overview page
    Refine potential matches: where we determine which ones actually match and return the summary documents for each location to build the MonitorSummary object
    Get monitor histograms: where we calculate the histograms for each monitor. In the future we might make this a separate API call.

This improves the overall code structure, and leaves the test coverage about the same depending on how you look at it. I think we can do more to improve the quality of code / tests here, but this seemed like a good initial place to draw the line for now.

In perfunctory testing on our internal observability clusters I saw perf improve from 2.5s to 1.1s on the Uptime homepage with no filters. So, it looks like there are potentially perf improvements (no real benchmarking was done).

Previously, this returned all pings from the latest check group. This was not actually used anywhere, only the summary pings are required for the current UI, so we now only return those from the list API as this saves a query.
@zube zube bot added [zube]: Done and removed [zube]: Ready labels Jul 9, 2020
andrewvc added a commit that referenced this issue Jul 9, 2020
Resolves #68096 , removing unnecessary scripted metric query from overview page and unifying the Check and Ping types.

This simplifies the types in a number of ways, and reduces the total quantity of code to execute the queries for the overview page. It also fixes the Tls and related types which were inconsistent and presented a problem here since they are used by this JS. There are now three stages where before there were four:

    Find potential matches: where we determine which monitor IDs are eligible for the overview page
    Refine potential matches: where we determine which ones actually match and return the summary documents for each location to build the MonitorSummary object
    Get monitor histograms: where we calculate the histograms for each monitor. In the future we might make this a separate API call.

This improves the overall code structure, and leaves the test coverage about the same depending on how you look at it. I think we can do more to improve the quality of code / tests here, but this seemed like a good initial place to draw the line for now.

In perfunctory testing on our internal observability clusters I saw perf improve from 2.5s to 1.1s on the Uptime homepage with no filters. So, it looks like there are potentially perf improvements (no real benchmarking was done).

Previously, this returned all pings from the latest check group. This was not actually used anywhere, only the summary pings are required for the current UI, so we now only return those from the list API as this saves a query.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability technical debt Improvement of the software architecture and operational architecture
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants