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: create tag-explorer page for analyzing tag breakdowns #1293

Merged
merged 50 commits into from
Jul 29, 2022

Conversation

dogfrogfog
Copy link
Contributor

@github-actions
Copy link
Contributor

github-actions bot commented Jul 21, 2022

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
webapp/public/assets/app.js 410.98 KB (+2.79% 🔺) 8.3 s (+2.79% 🔺) 3.2 s (-18.37% 🔽) 11.4 s
webapp/public/assets/app.css 13.36 KB (+3.58% 🔺) 268 ms (+3.58% 🔺) 0 ms (+100% 🔺) 268 ms
webapp/public/assets/styles.css 9.8 KB (0%) 197 ms (0%) 0 ms (+100% 🔺) 197 ms
packages/pyroscope-flamegraph/dist/index.js 90.04 KB (+0.05% 🔺) 1.9 s (+0.05% 🔺) 1.2 s (-4.61% 🔽) 3 s
packages/pyroscope-flamegraph/dist/index.node.js 81.37 KB (+0.05% 🔺) 1.7 s (+0.05% 🔺) 635 ms (+16.62% 🔺) 2.3 s
packages/pyroscope-flamegraph/dist/index.css 5.81 KB (0%) 117 ms (0%) 0 ms (+100% 🔺) 117 ms

@codecov
Copy link

codecov bot commented Jul 21, 2022

Codecov Report

Merging #1293 (b97817f) into main (c163b37) will decrease coverage by 0.95%.
The diff coverage is 42.15%.

@@            Coverage Diff             @@
##             main    #1293      +/-   ##
==========================================
- Coverage   68.52%   67.58%   -0.94%     
==========================================
  Files         112      119       +7     
  Lines        3713     3960     +247     
  Branches      842      906      +64     
==========================================
+ Hits         2544     2676     +132     
- Misses       1167     1282     +115     
  Partials        2        2              
Impacted Files Coverage Δ
webapp/javascript/util/updateRequests.ts 13.34% <14.29%> (-1.48%) ⬇️
webapp/javascript/services/render.ts 24.00% <22.23%> (-6.55%) ⬇️
webapp/javascript/util/appendLabelToQuery.ts 23.08% <23.08%> (ø)
webapp/javascript/redux/reducers/continuous.ts 26.23% <35.56%> (-0.71%) ⬇️
packages/pyroscope-models/src/groups.ts 100.00% <100.00%> (ø)
packages/pyroscope-models/src/index.ts 80.00% <100.00%> (+3.08%) ⬆️
webapp/javascript/components/Sidebar.tsx 86.91% <100.00%> (-0.90%) ⬇️
webapp/javascript/pages/constants.ts 100.00% <100.00%> (ø)
webapp/javascript/pages/math.ts 100.00% <100.00%> (ø)
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c163b37...b97817f. Read the comment docs.

@dogfrogfog
Copy link
Contributor Author

2022-07-22.21.13.19.mov

@dogfrogfog
Copy link
Contributor Author

/create-server

@Rperry2174
Copy link
Contributor

Rperry2174 commented Jul 28, 2022

two more request -- currently when you select the group by dropdown, nothing happens by default and it's not clear that you're supposed to be looking at a flamegraph. Let's fix that by defaulting to the first tag being selected in the table

image

Also lets call it "Tag explorer" in the sidebar and nest it first under the "continuous" section

@dogfrogfog
Copy link
Contributor Author

/create-server

@Rperry2174
Copy link
Contributor

Rperry2174 commented Jul 28, 2022

Looks great... Although another idea here :)

Can we also make it so that if no tag is selected, then we highlight the "group by" dropdown with this orange border:
image

I think that will make it clearer to the user that they should start by selecting a tag in that dropdown.

Once a tag is selected then the dropdown should look normal
image

Also lets replace the icon with a magnifying glass icon like this one: https://fontawesome.com/icons/magnifying-glass-chart?s=solid (if we can get with our version -- otherwise just use the next closest icon that we have access to)

@dogfrogfog dogfrogfog marked this pull request as ready for review July 29, 2022 12:29
Copy link
Contributor

@Rperry2174 Rperry2174 left a comment

Choose a reason for hiding this comment

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

lgtm! thanks @dogfrogfog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants