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] Use manual intervals for ping histogram #72928

Merged
merged 4 commits into from
Jul 24, 2020

Conversation

andrewvc
Copy link
Contributor

@andrewvc andrewvc commented Jul 22, 2020

Fixes elastic/uptime#215

Prior to this we'd get too few buckets in some ranges.

Fixes elastic/uptime#215

Prior to this we'd get too few buckets in some ranges.
@andrewvc andrewvc added bug Fixes for quality problems that affect the customer experience Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.9.0 labels Jul 22, 2020
@andrewvc andrewvc requested a review from justinkambic July 22, 2020 17:57
@andrewvc andrewvc requested a review from a team as a code owner July 22, 2020 17:57
@andrewvc andrewvc self-assigned this Jul 22, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@justinkambic
Copy link
Contributor

Could we be dropping the most recent bucket?

image

We are getting a bucket in the result set:

image

It looks like you haven't touched the client code, so this could be a separate bug.

@andrewvc
Copy link
Contributor Author

@justinkambic I think that's unrelated. I just fixed the tests, they did some unnecessarily specific assertions about bucket size. IMHO that's not necessary.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

Copy link
Contributor

@justinkambic justinkambic left a comment

Choose a reason for hiding this comment

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

LGTM - one item that we can do as a follow-up if you agree it's worthwhile.

Tested locally as well and seems to be working.

@@ -63,7 +48,12 @@ export const getPingHistogram: UMElasticsearchQueryFn<
size: 0,
aggs: {
timeseries: {
...seriesHistogram,
date_histogram: {
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing that might be nice is to add two tests to ensure that the query we generate is providing the expected fixed_interval value. We have "test coverage" here, but I don't think the params are ever validated by a test. I could write this test in a follow-up, I don't think we need to do it here.

@andrewvc andrewvc merged commit c2ad4bf into elastic:master Jul 24, 2020
@andrewvc andrewvc deleted the use-manual-buckets-ping-history branch July 24, 2020 01:02
andrewvc added a commit to andrewvc/kibana that referenced this pull request Jul 24, 2020
* [Uptime] Use manual intervals for ping histogram

Fixes elastic/uptime#215

Prior to this we'd get too few buckets in some ranges.

* Update test fixtures, remove overly-specific checks

* Remove unused import
andrewvc added a commit to andrewvc/kibana that referenced this pull request Jul 24, 2020
* [Uptime] Use manual intervals for ping histogram

Fixes elastic/uptime#215

Prior to this we'd get too few buckets in some ranges.

* Update test fixtures, remove overly-specific checks

* Remove unused import
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 27, 2020
* master: (111 commits)
  Remove flaky note from gauge tests (elastic#73240)
  Convert functional vega tests to ts and unskip tests (elastic#72238)
  [Graph] Unskip graph tests (elastic#72291)
  Add default Elasticsearch credentials to docs (elastic#72617)
  [APM] Read body from indicesStats in upload-telemetry-data (elastic#72732)
  The directory in the command was missing the /generated directory and would cause all definitions to be regenerated in the wrong place. (elastic#72766)
  [KP] use new ES client in SO service (elastic#72289)
  [Security Solution][Exceptions] Prevents value list entries from co-existing with non value list entries (elastic#72995)
  Return EUI CSS to Shareable Runtime (elastic#72990)
  Removed useless karma test (elastic#73190)
  [INGEST_MANAGER] Make package config name blank for endpoint on Package Config create (elastic#73082)
  [Ingest Manager] Support DEGRADED state in fleet agent event (elastic#73104)
  [Security Solution][Detections] Change detections breadcrumb title (elastic#73059)
  [ML] Fixing unnecessary deleting job polling (elastic#73087)
  [ML] Fixing recognizer wizard create job button (elastic#73025)
  [Composable template] Preview composite template (elastic#72598)
  [Uptime] Use manual intervals for ping histogram (elastic#72928)
  [Security Solution][Endpoint] Task/policy save modal text change, remove duplicate policy details text (elastic#73130)
  [Maps] fix tile layer attibution text and attribution link validation errors (elastic#73160)
  skip ingest pipeline api tests
  ...
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jul 28, 2020
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

andrewvc added a commit that referenced this pull request Jul 29, 2020
Fixes elastic/uptime#215

Prior to this we'd get too few buckets in some ranges.
andrewvc added a commit that referenced this pull request Jul 29, 2020
* [Uptime] Use manual intervals for ping histogram

Fixes elastic/uptime#215

Prior to this we'd get too few buckets in some ranges.

* Update test fixtures, remove overly-specific checks

* Remove unused import

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jul 29, 2020
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:fix Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Uptime] Switch away from Auto-date histogram
4 participants