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

[APM] Use rounded bucket sizes for transaction distribution #42830

Merged
merged 2 commits into from
Aug 8, 2019

Conversation

sorenlouv
Copy link
Member

We've gotten some feedback that the bucket sizes (interval) on the transaction distribution viz are odd - I decided to take a quick stab at it and I think it's a good improvement.

Before: tick marks are not aligned with buckets, and the bucket values are arbitrary
Screen Shot 2019-08-07 at 11 18 33

After: tick marks are aligned with buckets, and the bucket values are rounded
Screen Shot 2019-08-07 at 11 18 44

Implications:
We no longer show a fixed number of buckets. Instead we aim for the targetBucketSize and show a dynamic number of buckets.

@sorenlouv sorenlouv added the Team:APM - DEPRECATED Use Team:obs-ux-infra_services. label Aug 7, 2019
@sorenlouv sorenlouv requested a review from a team as a code owner August 7, 2019 13:21
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui

@@ -58,7 +58,7 @@ export function bucketFetcher(
min_doc_count: 0,
extended_bounds: {
min: 0,
max: bucketSize * bucketTargetCount
max: distributionMax
Copy link
Member Author

Choose a reason for hiding this comment

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

The previous max value was calculated from the rounded bucketSize so it was not accurate and would therefore miss buckets. Using the exact distributionMax from the stats request fixes this.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

* you may not use this file except in compliance with the Elastic License.
*/

export function roundNice(v: number) {
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this file could use a more descriptive name (maybe roundToNearestFiveOrTen?) and v could be something like value. Maybe a comment w/ some examples as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I did add a test which I feel is fairly descriptive but adding examples as comments directly next to the implementation will be an improvement.

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.

LGTM, suggested one possible improvement for readability.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sorenlouv sorenlouv merged commit 0517ec9 into elastic:master Aug 8, 2019
@sorenlouv sorenlouv deleted the rounded-bucket-size branch August 8, 2019 20:10
sorenlouv added a commit that referenced this pull request Aug 8, 2019
…42978)

* [APM] Use rounded bucket sizes for transaction distribution

* Rename and add examples
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 9, 2019
…p-metrics-selectall

* 'master' of github.com:elastic/kibana: (306 commits)
  [ML] Adding job overrides to the module setup endpoint (elastic#42946)
  [APM] Fix missing RUM url (elastic#42940)
  close socket timeouts without message (elastic#42456)
  Upgrade elastic/charts to 8.1.6 (elastic#42518)
  [ML] Delete old AngularJS data visualizer and refactor folders (elastic#42962)
  Add custom formatting for Date Nanos Format (elastic#42445)
  [Vega] Shim new platform - vega_fn.js -> vega_fn.js , use ExpressionFunction (elastic#42582)
  add socket.getPeerCertificate to KibanaRequest (elastic#42929)
  [Automation] ISTANBUL PRESET PATH is not working fine with constructor(private foo) (elastic#42683)
  [ML] Data frames: Updated stats structure. (elastic#42923)
  [Code] fixed the issue that the repository can not be deleted in some cases. (elastic#42841)
  [kbn-es] Support for passing regex value to ES (elastic#42651)
  Connect to Elasticsearch via SSL when starting kibana with `--ssl` (elastic#42840)
  Add Elasticsearch SSL support for integration tests (elastic#41765)
  Fix duplicate fetch in Visualize (elastic#41204)
  [DOCS] TSVB and Timelion clean up (elastic#42953)
  [Maps] [File upload] Fix maps geojson upload hanging on index step (elastic#42623)
  [APM] Use rounded bucket sizes for transaction distribution (elastic#42830)
  [yarn.lock] consistent resolve domain (elastic#42969)
  [Uptime] [Test] Repurpose unit test assertions to avoid flakiness (elastic#40650)
  ...
sorenlouv added a commit to sorenlouv/kibana that referenced this pull request Aug 10, 2019
…42830)

* [APM] Use rounded bucket sizes for transaction distribution

* Rename and add examples
@sorenlouv sorenlouv removed their assignment Sep 5, 2019
@dgieselaar
Copy link
Member

@sqren This actually looks kind of weird for minutes:

image

bug?

@sorenlouv
Copy link
Member Author

sorenlouv commented Sep 10, 2019

@dgieselaar yeah, that does look weird. Since the bucket size is always in milliseconds that's what I based it on, but clearly that doesn't work when the client converts it to minutes 🤔

It's not a blocker for 7.4 but feel free to open an issue for it. We already have this issue for error occurrence histogram: #43503

@dgieselaar
Copy link
Member

I'll just add this one to that issue, looks like they roughly require the same work.

@dgieselaar
Copy link
Member

Maybe not though 😅 I'll open a new one.

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.

4 participants