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

feat: show percentages for diff table instead of absolute values #1697

Merged
merged 10 commits into from
Nov 17, 2022

Conversation

dogfrogfog
Copy link
Contributor

@dogfrogfog dogfrogfog commented Nov 10, 2022

Brief

Changes

  • change diff table values from absolute to percent

demo link: https://pr-1697.pyroscope.io/comparison-diff?query=rideshare-app-golang.alloc_objects%7B%7D&rightQuery=rideshare-app-golang.alloc_objects%7B%7D&leftQuery=rideshare-app-golang.alloc_objects%7B%7D

Screen.Recording.2022-11-10.at.13.52.08.mov

Concerns

  • im using .toFixed(2) and for some values that looks strange

image

i think we can either show smth like < 0.01 % like we do with absolute values or increase number of digits after comma

image

@github-actions
Copy link
Contributor

github-actions bot commented Nov 10, 2022

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
webapp/public/assets/app.js 492.19 KB (-0.13% 🔽) 9.9 s (-0.13% 🔽) 3.7 s (-2.14% 🔽) 13.6 s
webapp/public/assets/app.css 19.92 KB (-0.3% 🔽) 399 ms (-0.3% 🔽) 0 ms (+100% 🔺) 399 ms
webapp/public/assets/styles.css 9.56 KB (+0.02% 🔺) 192 ms (+0.02% 🔺) 0 ms (+100% 🔺) 192 ms
packages/pyroscope-flamegraph/dist/index.js 131.26 KB (-0.54% 🔽) 2.7 s (-0.54% 🔽) 1.8 s (+23.02% 🔺) 4.4 s
packages/pyroscope-flamegraph/dist/index.node.js 131.95 KB (-0.51% 🔽) 2.7 s (-0.51% 🔽) 814 ms (+49.37% 🔺) 3.5 s
packages/pyroscope-flamegraph/dist/index.css 8.29 KB (-0.77% 🔽) 166 ms (-0.77% 🔽) 0 ms (+100% 🔺) 166 ms

@codecov
Copy link

codecov bot commented Nov 10, 2022

Codecov Report

Base: 66.22% // Head: 66.00% // Decreases project coverage by -0.23% ⚠️

Coverage data is based on head (c202255) compared to base (8d92b0a).
Patch coverage: 53.20% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1697      +/-   ##
==========================================
- Coverage   66.22%   66.00%   -0.22%     
==========================================
  Files         164      168       +4     
  Lines        5441     5519      +78     
  Branches     1220     1241      +21     
==========================================
+ Hits         3603     3642      +39     
- Misses       1831     1868      +37     
- Partials        7        9       +2     
Impacted Files Coverage Δ
...e-flamegraph/src/FlameGraph/FlameGraphRenderer.tsx 49.70% <ø> (-<0.01%) ⬇️
...kages/pyroscope-flamegraph/src/Toolbar.module.scss 61.54% <ø> (ø)
packages/pyroscope-flamegraph/src/Toolbar.tsx 80.38% <ø> (-2.83%) ⬇️
.../pyroscope-flamegraph/src/Tooltip/TableTooltip.tsx 47.28% <35.72%> (-10.86%) ⬇️
...ackages/pyroscope-flamegraph/src/ProfilerTable.tsx 56.37% <55.18%> (-4.53%) ⬇️
...scope-flamegraph/src/Tooltip/FlamegraphTooltip.tsx 76.39% <100.00%> (ø)
packages/pyroscope-flamegraph/src/format/format.ts 73.22% <100.00%> (+0.25%) ⬆️
... and 14 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dogfrogfog
Copy link
Contributor Author

/create-server

@dogfrogfog dogfrogfog changed the title change absolute values to percent from flamebearer.numTicks feat: show percentages for diff table instead of absolute values Nov 10, 2022
@dogfrogfog dogfrogfog marked this pull request as ready for review November 10, 2022 13:54
@Ivanikitin Ivanikitin added the frontend Mostly JS code label Nov 10, 2022
@dogfrogfog
Copy link
Contributor Author

/create-server

@dogfrogfog
Copy link
Contributor Author

Screen.Recording.2022-11-11.at.16.16.40.mov

@petethepig
Copy link
Member

/create-server

@Rperry2174
Copy link
Contributor

I think this looks great... I think we should just make the table have the exact same 3 columns as the tooltip has :

image

Let's remove the 3 button options for now
The table should have:

  • total (left) -> Baseline
  • total (right) -> Comparison
  • Add a column for diff

wdyt @petethepig ?

@grafana grafana deleted a comment from pyroscopebot Nov 12, 2022
@petethepig
Copy link
Member

@Rperry2174 yeah that makes sense

@dogfrogfog
Copy link
Contributor Author

/create-server

@dogfrogfog
Copy link
Contributor Author

Screenshot 2022-11-14 at 16 54 41

@Rperry2174
Copy link
Contributor

Rperry2174 commented Nov 14, 2022

Looks great.. lets also show the same data that shows on the flamegraph over the table as well for the tooltip:
image

vs

image

@dogfrogfog
Copy link
Contributor Author

/create-server

@dogfrogfog
Copy link
Contributor Author

Screen.Recording.2022-11-15.at.13.39.05.mov

@dogfrogfog
Copy link
Contributor Author

/create-server

Copy link
Collaborator

@eh-am eh-am left a comment

Choose a reason for hiding this comment

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

Just to be sure, are we getting rid of these?
image

cc @Rperry2174

packages/pyroscope-models/src/flamebearer.ts Outdated Show resolved Hide resolved
packages/pyroscope-flamegraph/src/format/format.ts Outdated Show resolved Hide resolved
@dogfrogfog
Copy link
Contributor Author

yes, i removed code for this buttons

@Rperry2174
Copy link
Contributor

Just to be sure, are we getting rid of these? image

cc @Rperry2174

yes I think we should get rid of them (tbh many don't know what they actually mean anyway)

@Rperry2174
Copy link
Contributor

/create-server

@dogfrogfog
Copy link
Contributor Author

/create-server

@Rperry2174 Rperry2174 merged commit 71efcb8 into main Nov 17, 2022
@Rperry2174 Rperry2174 deleted the feature/percentages-for-diff-table branch November 17, 2022 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Mostly JS code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants