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

Flamegraph: Add switch for color scheme by value or by package #70770

Merged
merged 7 commits into from Jun 30, 2023

Conversation

aocenas
Copy link
Member

@aocenas aocenas commented Jun 27, 2023

Adds a button with dropdown to select how the items in flamegraph should be colored.

The current value representation is a bit minimalistic, by changing the gradient in the dot in the button but the color dot is what we use as a color picker so it is sort of consistent with other pickers.

flamegraph_colors_cheme.mp4

Note:
This is behind feature flag flameGraphV2

/* eslint-disable no-bitwise */
/* eslint-disable camelcase */

export default function murmurhash3_32_gc(key: string, seed = 0) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Taken from pyro code, not sure if we need this, the hash func quality here probably isn't the most important for color coding.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we only need a hash can we use the same one from the color-generator?

// the language from the backend and use the right regex but right now we just try all of them from most to least
// specific.
const matchers = [
['phpspy', /^(?<packageName>(.*\/)*)(?<filename>.*\.php+)(?<line_info>.*)$/],

Check failure

Code scanning / CodeQL

Inefficient regular expression High

This part of the regular expression may cause exponential backtracking on strings containing many repetitions of '/'.
// specific.
const matchers = [
['phpspy', /^(?<packageName>(.*\/)*)(?<filename>.*\.php+)(?<line_info>.*)$/],
['pyspy', /^(?<packageName>(.*\/)*)(?<filename>.*\.py+)(?<line_info>.*)$/],

Check failure

Code scanning / CodeQL

Inefficient regular expression High

This part of the regular expression may cause exponential backtracking on strings containing many repetitions of '/'.
const matchers = [
['phpspy', /^(?<packageName>(.*\/)*)(?<filename>.*\.php+)(?<line_info>.*)$/],
['pyspy', /^(?<packageName>(.*\/)*)(?<filename>.*\.py+)(?<line_info>.*)$/],
['rbspy', /^(?<packageName>(.*\/)*)(?<filename>.*\.rb+)(?<line_info>.*)$/],

Check failure

Code scanning / CodeQL

Inefficient regular expression High

This part of the regular expression may cause exponential backtracking on strings containing many repetitions of '/'.
@aocenas aocenas marked this pull request as ready for review June 28, 2023 13:16
@aocenas aocenas requested a review from a team as a code owner June 28, 2023 13:16
@github-actions
Copy link
Contributor

Backend code coverage report for PR #70770
No changes

@github-actions
Copy link
Contributor

github-actions bot commented Jun 28, 2023

Frontend code coverage report for PR #70770

Plugin Main PR Difference
explore 86.57% 86.57% 0%

Copy link
Contributor

@joey-grafana joey-grafana left a comment

Choose a reason for hiding this comment

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

  • Do we want to do something about the code scanning alerts?
  • Clicking around focusing blocks seems to mess up the color scheme
Screen.Recording.2023-06-29.at.14.20.18.mov

/* eslint-disable no-bitwise */
/* eslint-disable camelcase */

export default function murmurhash3_32_gc(key: string, seed = 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we only need a hash can we use the same one from the color-generator?

@aocenas
Copy link
Member Author

aocenas commented Jun 30, 2023

@joey-grafana

If we only need a hash can we use the same one from the color-generator?

Would like to leave it for later. The hash function in traceView seems very simplistic (not that I am expert on hashFunctions) so not sure if it does not make sense to use some normal hash function for this so the hashes (and colors) are more randomly distributed. Will take a look at that at some point though.

Do we want to do something about the code scanning alerts?

If they do not block the PR would also leave it for now. The problem is I am not really sure how to improve that right away and the labels are mostly machine generated so it's not likely somebody imho that somebody would pass some very weird string here (and even so it will just be slow UI). Also this may possibly change a bit if we pass the language information from backend already.

Clicking around focusing blocks seems to mess up the color scheme

Can you try again? Does not happen for me and I remember fixing issue like this in this PR (not sure when) so wonder if you may been on some older commit or something like that.

Copy link
Contributor

@joey-grafana joey-grafana left a comment

Choose a reason for hiding this comment

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

Appears to have cleared itself up. All you seem to have done is pulled from main so not sure what was causing the issue in the video.

This looks brilliant btw 👍

@aocenas aocenas merged commit 3eaee12 into main Jun 30, 2023
12 of 14 checks passed
@aocenas aocenas deleted the aocenas/flamegraph/color-schemes branch June 30, 2023 15:01
@ricky-undeadcoders ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023
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.

None yet

4 participants