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

Use HDR for percentiles #64758

Merged
merged 2 commits into from
May 2, 2020
Merged

Conversation

sorenlouv
Copy link
Member

Closes #62399

This seems like a simple optimisation that we should do (previously discussed on Slack). Additional benchmarks would be nice but I'm not sure it's absolutely necessary.

@sorenlouv sorenlouv requested a review from a team as a code owner April 29, 2020 11:35
Copy link
Member

@dgieselaar dgieselaar left a comment

Choose a reason for hiding this comment

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

FWIW, you can't use this on negative values. When I was trying this out it would actually break on APM data on apm.elstc.co because the Ruby agent was inadvertently uploading some transactions with negative duration for a short amount of time. We might want to safeguard against that by adding a range query, but not sure if it's necessary.

@sorenlouv
Copy link
Member Author

sorenlouv commented Apr 29, 2020

We might want to safeguard against that by adding a range query, but not sure if it's necessary.

I thought that was due to an error with the Ruby agent and APM Server would discard negative latencies going forward? That sounds like the better solution to me at least.

What do you think @axw ?

@dgieselaar
Copy link
Member

@sqren it was a bug indeed and it seems like an unlikely scenario in any case.

@axw
Copy link
Member

axw commented Apr 29, 2020

I thought that was due to an error with the Ruby agent and APM Server would discard negative latencies going forward? That sounds like the better solution to me at least.

Sounds good to me. I've opened elastic/apm-server#3715 to track it.

@axw
Copy link
Member

axw commented Apr 30, 2020

Sorry 😬

@sorenlouv sorenlouv force-pushed the use-hdr-for-percentiles branch 2 times, most recently from 0423082 to c70146f Compare April 30, 2020 21:52
@sorenlouv sorenlouv force-pushed the use-hdr-for-percentiles branch from c70146f to 55a48e4 Compare April 30, 2020 22:17
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

  • 💚 Build #44636 succeeded c70146f70fca1e746a56dbb02641668c3fa2ac1a
  • 💚 Build #44388 succeeded 04230828086e905171aefdecde44cf44cb392f67
  • 💔 Build #44362 failed 6993e7931e475bde5f21368892716db89b2f827e
  • 💔 Build #44040 failed 6993e7931e475bde5f21368892716db89b2f827e

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

@sorenlouv sorenlouv merged commit d314e46 into elastic:master May 2, 2020
@sorenlouv sorenlouv deleted the use-hdr-for-percentiles branch May 2, 2020 20:12
v1v added a commit to v1v/kibana that referenced this pull request May 4, 2020
* upstream/master: (315 commits)
  [APM] Fix failing `ApmIndices` test (elastic#64965)
  [APM] Fix paths for ts optimization script (elastic#65012)
  Use HDR for percentiles (elastic#64758)
  [EPM] fix updates available filter (elastic#64957)
  [Uptime] Certificates page (elastic#64059)
  load lens app lazily (elastic#64769)
  [legacy/server/config] remove unnecessary deps for simple helper (elastic#64954)
  Fixed alert Edit flyout shows the error message when one of this actions has a preconfigured action type (elastic#64742)
  [data.search.aggs] Remove legacy aggs APIs. (elastic#64719)
  Fixed `AddAlert` flyout does not immediately update state to reflect new props (elastic#64927)
  [Discover] Show doc viewer action buttons on focus (elastic#64912)
  [EPM] restrict package install endpoint from installing/updating to old packages (elastic#64932)
  [Metrics UI] Add inventory metric threshold alerts (elastic#64292)
  [Canvas] Adds edit menu (elastic#64738)
  [Canvas] Adds refresh and autoplay options to view menu (elastic#64375)
  [Lens] Trigger a filter action on click in datatable visualization (elastic#63840)
  [SIEM][CASE] Refactor Connectors - Jira Connector (elastic#63450)
  [APM] Client new platform migration (elastic#64046)
  [Monitoring] NP Migration complete client cutover (elastic#62908)
  Ingest Node Pipelines UI (elastic#62321)
  ...
jloleysens added a commit to jloleysens/kibana that referenced this pull request May 4, 2020
…or-part-mvp-2

* 'master' of github.com:elastic/kibana: (51 commits)
  [APM] Fix failing `ApmIndices` test (elastic#64965)
  [APM] Fix paths for ts optimization script (elastic#65012)
  Use HDR for percentiles (elastic#64758)
  [EPM] fix updates available filter (elastic#64957)
  [Uptime] Certificates page (elastic#64059)
  load lens app lazily (elastic#64769)
  [legacy/server/config] remove unnecessary deps for simple helper (elastic#64954)
  Fixed alert Edit flyout shows the error message when one of this actions has a preconfigured action type (elastic#64742)
  [data.search.aggs] Remove legacy aggs APIs. (elastic#64719)
  Fixed `AddAlert` flyout does not immediately update state to reflect new props (elastic#64927)
  [Discover] Show doc viewer action buttons on focus (elastic#64912)
  [EPM] restrict package install endpoint from installing/updating to old packages (elastic#64932)
  [Metrics UI] Add inventory metric threshold alerts (elastic#64292)
  [Canvas] Adds edit menu (elastic#64738)
  [Canvas] Adds refresh and autoplay options to view menu (elastic#64375)
  [Lens] Trigger a filter action on click in datatable visualization (elastic#63840)
  [SIEM][CASE] Refactor Connectors - Jira Connector (elastic#63450)
  [APM] Client new platform migration (elastic#64046)
  [Monitoring] NP Migration complete client cutover (elastic#62908)
  Ingest Node Pipelines UI (elastic#62321)
  ...

# Conflicts:
#	x-pack/plugins/ingest_pipelines/common/types.ts
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_fields.tsx
#	x-pack/plugins/ingest_pipelines/public/shared_imports.ts
gmmorris added a commit to gmmorris/kibana that referenced this pull request May 4, 2020
* master: (44 commits)
  onEvent prop for expression component (elastic#64995)
  [APM] Fix failing `ApmIndices` test (elastic#64965)
  [APM] Fix paths for ts optimization script (elastic#65012)
  Use HDR for percentiles (elastic#64758)
  [EPM] fix updates available filter (elastic#64957)
  [Uptime] Certificates page (elastic#64059)
  load lens app lazily (elastic#64769)
  [legacy/server/config] remove unnecessary deps for simple helper (elastic#64954)
  Fixed alert Edit flyout shows the error message when one of this actions has a preconfigured action type (elastic#64742)
  [data.search.aggs] Remove legacy aggs APIs. (elastic#64719)
  Fixed `AddAlert` flyout does not immediately update state to reflect new props (elastic#64927)
  [Discover] Show doc viewer action buttons on focus (elastic#64912)
  [EPM] restrict package install endpoint from installing/updating to old packages (elastic#64932)
  [Metrics UI] Add inventory metric threshold alerts (elastic#64292)
  [Canvas] Adds edit menu (elastic#64738)
  [Canvas] Adds refresh and autoplay options to view menu (elastic#64375)
  [Lens] Trigger a filter action on click in datatable visualization (elastic#63840)
  [SIEM][CASE] Refactor Connectors - Jira Connector (elastic#63450)
  [APM] Client new platform migration (elastic#64046)
  [Monitoring] NP Migration complete client cutover (elastic#62908)
  ...
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it 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 May 5, 2020
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

sorenlouv added a commit that referenced this pull request May 6, 2020
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[APM] investigate using HDR for percentiles aggregations
4 participants