pkg/query: relative comparisons #4578
Conversation
pkg/query/columnquery.go
Outdated
| diff := multiplyInt64By(mem, columns[len(columns)-4].(*array.Int64), -1*cumulativeRatio) | ||
| defer diff.Release() | ||
| diffPerSecond := multiplyFloat64By(mem, columns[len(columns)-3].(*array.Float64), -1) | ||
| diffPerSecond := multiplyFloat64By(mem, columns[len(columns)-3].(*array.Float64), -1*cumulativePerSecondRatio) |
There was a problem hiding this comment.
Right now, only this side of the comparison is changed. However, if this side has the higher total cumulative value, we need to "scale" it down. Meaning that we sometimes calculate new values with 1 * 0.95 which results in 0 for the int64 columns.
In that case, we probably should instead "scale" the other side up, right? Basically making it a calculation of 1 * 1.05 which would still be 1 once casted back to int64.
metalmatze
force-pushed
the
compare-relative
branch
from
c823185
to
8874a71
Compare
|
🤖 Meticulous spotted visual differences in 1 of 434 screens tested: view and approve differences detected. Last updated for commit bc2754a. This comment will update as new commits are pushed. |
metalmatze
changed the title
|
Now, we need to figure out the UX and UI to enable or disable the relative comparison. |
metalmatze
force-pushed
the
compare-relative
branch
from
8874a71
to
e9a4114
Compare
When comparing profiles, people usually want to compare relatively, meaning they care about the relative percentages that changed between the profiles, not about the exact times a stack has been seen. This change ensures that we scale both profiles to the same total cumulative value for the root and then multiply each diff value by the ratio between both roots.
metalmatze
force-pushed
the
compare-relative
branch
from
3f18395
to
bc2754a
Compare
There was a problem hiding this comment.
This largely looks good except for one small comment, but I'm actually more wondering about the UX of this, as we have plenty of evidence that suggests that people want to do both absolute and relative comparisons, depending on the situation, so we should allow users to do both, but with this patch we'd only allow them to do relative comparisons.
| } else { | ||
| cumulativeCompareRatio = float64(totalBaseValue) / float64(totalCompareValue) | ||
| } | ||
| if totalCompareValuePerSecond > totalBaseValuePerSecond { |
There was a problem hiding this comment.
Having two if statements here doesn't make sense to me, that would mean we can have a situation where we're scaling cumulative based on one side and per second on the other. We should always scale the same side.
When comparing profiles, people usually want to compare relatively, meaning they care about the relative percentages that changed between the profiles, not about the exact times a stack has been seen.
This change ensures that we scale both profiles to the same total cumulative value for the root and then multiply each diff value by the ratio between both roots.