Skip to content

[APM] Transaction duration chart always shows duration in ms #42375

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 6, 2019
Merged

[APM] Transaction duration chart always shows duration in ms #42375

merged 3 commits into from
Aug 6, 2019

Conversation

@bartvanremortele
Copy link
Contributor

@bartvanremortele bartvanremortele commented Jul 31, 2019 •
edited by sorenlouv
Loading

Summary

Fixes #42002

This fixes the displaying of the transaction chart's ticks and tooltip. It formats the values to the appropriate unit. I looked into possibly solving this by calling the tick formatter with the max value using d3.max() but this would require changes to be made on y-axis, axis and axis-ticks inside the react-vis library.

Schermafbeelding 2019-07-31 om 18 37 33
Schermafbeelding 2019-07-31 om 18 39 17

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@bartvanremortele bartvanremortele requested a review from a team as a code owner July 31, 2019 16:57
@elasticmachine
Copy link
Contributor

elasticmachine commented Jul 31, 2019

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@bartvanremortele
Copy link
Contributor Author

bartvanremortele commented Jul 31, 2019

@sqren there are two potential issues remaining:

  • formatting the legend's average value
  • formatting the transactions in the table below the chart

Schermafbeelding 2019-07-31 om 18 58 59
Schermafbeelding 2019-07-31 om 18 59 05

@lukeelmers lukeelmers added the Team:APM - DEPRECATED Use Team:obs-ux-infra_services. label Aug 2, 2019
@elasticmachine
Copy link
Contributor

elasticmachine commented Aug 2, 2019

Pinging @elastic/apm-ui

sorenlouv
sorenlouv reviewed Aug 6, 2019
View reviewed changes
Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

Thanks again @bartvanremortele! This looks great, just a few minor comments.

@bartvanremortele
Copy link
Contributor Author

bartvanremortele commented Aug 6, 2019

@sqren Ready for review again. I'm interested to pick up more if the other issues should also be fixed.

@sorenlouv
Copy link
Member

sorenlouv commented Aug 6, 2019

retest

@elasticmachine
Copy link
Contributor

elasticmachine commented Aug 6, 2019

💚 Build Succeeded

@sorenlouv sorenlouv merged commit 98b445a into elastic:master Aug 6, 2019
@sorenlouv
Copy link
Member

sorenlouv commented Aug 6, 2019

Thanks again @bartvanremortele! Will you create the backport to 7.x?

@bartvanremortele
Copy link
Contributor Author

bartvanremortele commented Aug 6, 2019

Sure! I will do it an hour or so

@bartvanremortele bartvanremortele mentioned this pull request Aug 6, 2019
Merged
bartvanremortele added a commit to bartvanremortele/kibana that referenced this pull request Aug 6, 2019
@bartvanremortele
[APM] Transaction duration chart always shows duration in ms (elast…
df2aff0 
…ic#42375)

* make the transaction chart use the appropriate unit depending on the max value

* fix TimeFormatter type not being imported
sorenlouv pushed a commit that referenced this pull request Aug 6, 2019
@bartvanremortele @sorenlouv
[APM] Transaction duration chart always shows duration in ms (#42375)…
02d14f9 
… (#42759)

* make the transaction chart use the appropriate unit depending on the max value

* fix TimeFormatter type not being imported
@dgieselaar
Copy link
Member

dgieselaar commented Sep 10, 2019

Verified that unit is adjusted based on max y value.

@dgieselaar dgieselaar self-assigned this Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sorenlouv sorenlouv sorenlouv approved these changes

Assignees

@dgieselaar dgieselaar

Labels

release_note:enhancement review Team:APM - DEPRECATED Use Team:obs-ux-infra_services. v7.4.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[APM] Transaction duration chart always shows duration in ms

5 participants

@bartvanremortele @elasticmachine @sorenlouv @dgieselaar @lukeelmers