*: Flamegraph data trimming #2299
Conversation
| @@ -224,6 +224,9 @@ message QueryRequest { | |||
|
|
|||
| // filter_query is the query string to filter the profile samples | |||
| optional string filter_query = 6; | |||
|
|
|||
| // disable_trimming disables trimming of insignificant nodes in the report | |||
| optional bool disable_trimming = 7; | |||
There was a problem hiding this comment.
if we allow disabling it in the API at all, then I think we should allow passing the exact parameters to trim with
There was a problem hiding this comment.
Yeah right, you mean the NodeCutOffFraction value, right?
There was a problem hiding this comment.
What if we always set the node_trim_threshold in the UI and if it's unset (or the default value of 0.0) then it's disabled?
There was a problem hiding this comment.
Yes, that sounds simpler. Will do that.
…ean and hooked in NodeCutOffThreshold prefrence for the query request
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
pkg/query/flamegraph_table.go
Outdated
| if len(graph.Root.Children) > 0 { | ||
| newTotal = children.Cumulative() | ||
| newDiff = children.Diff() | ||
| } else { |
There was a problem hiding this comment.
I don't understand this else statement. If there are no children in the root node it should return 0 anyway, so this else should not be needed?
There was a problem hiding this comment.
Hmm. True, I was using this for the FlamegraphNode type where if there is no Children would mean leaf node and will have to take its own value. Sounds unnecessary for the Root node, will make it 0.
| newTotal := int64(0) | ||
| newDiff := int64(0) | ||
| if len(graph.Root.Children) > 0 { | ||
| newTotal = children.Cumulative() |
There was a problem hiding this comment.
I think we should include the difference in cumulative due to node trimming in the proto response. I think it would be very confusing to click on a sample on a graph with let's say 100 CPU samples, and then only see 85 cumulative in the root node.
There was a problem hiding this comment.
In that case, how do we account for the difference in the Child nodes? As the sum of child nodes would still only be 85 right?
There was a problem hiding this comment.
Sorry I meant ultimately the UI should show something like "Showing 85 out of 100 samples"
There was a problem hiding this comment.
So actually I meant that we should include the previous and trimmed total value in the response
There was a problem hiding this comment.
Is samples the right terminology for all profile types?
Also would it be the right place to show this message or should we hide it under some i icon?
| @@ -224,6 +224,9 @@ message QueryRequest { | |||
|
|
|||
| // filter_query is the query string to filter the profile samples | |||
| optional string filter_query = 6; | |||
|
|
|||
| // disable_trimming disables trimming of insignificant nodes in the report | |||
| optional bool disable_trimming = 7; | |||
There was a problem hiding this comment.
What if we always set the node_trim_threshold in the UI and if it's unset (or the default value of 0.0) then it's disabled?
| // disable_trimming disables trimming of insignificant nodes in the report | ||
| optional bool disable_trimming = 7; | ||
|
|
||
| // node_trim_threshold is the threshold for trimming insignificant nodes in the report |
There was a problem hiding this comment.
From this comment it is unclear to me that this is a fraction or percentage. By this comment I would expect this to be a total value, which I don't think is true.
| disabled={disabled} | ||
| /> | ||
| ); | ||
| case 'number': |
There was a problem hiding this comment.
interesting idea, but I think this would break shareability, if you have a different configuration from me and you share a link with me I will see different data, if we expose this value (which I don't think we should today), then we should make it part of exploring the data (and therefore part of the URL)
There was a problem hiding this comment.
Yeah ok, and we can make this a value added to the URL whenever the user is in the ProfileExplorer and the query formed with the value from the URL taking higher precedence over the value from the preference so that the shareability is maintained.
But I would prefer moving this into a separate issue and implement it once #2115 is merged. As it has some nice utilities for dealing with the URL params state. Any concerns?
There was a problem hiding this comment.
I would suggest then let's just use a hard-coded value for a start. (I'm not even sure it ever doesn't need to be)
There was a problem hiding this comment.
I tried to see if I can implement the URL state without that PR, but it looks like a lot of duplicate work between these two. So, I'll hold it off.
So, if I understand it correctly, meanwhile, you would like this option removed from the user preference and work with just the hardcoded 0.005cut-off value?
- [ ] Garbage collect the values corresponding to the trimmed nodes from the tablesFixes #2269