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 37 of 498 screens tested: view and approve differences detected. Last updated for commit 3f4cb5c. 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
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.
pkg/query/columnquery.go
Outdated
| } 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.
metalmatze
force-pushed
the
compare-relative
branch
from
bc2754a
to
e42fc4f
Compare
This indicates whether or not to use absolute or relative profile comparison.
Additionally, adding a new test that tests if the relative works too
This now sums up the total cumulative value of each side before scaling up the smaller side to match the overall cumulative value of the bigger side.
… comparison For delta profiles we want to use relative comparisons by default, whereas for non-delta profiles, we want to use absolute comparisons by default.
AbsoluteThis shows the absolute comparison between the two profiles. RelativeThis shows the relative comparison of the two profiles. The smaller (in terms of overall cumulative value) profile gets scaled up (all of its values are multiplied by the factor the bigger profile is bigger) to have both profiles' total cumulative values be the same. Therefore, compare the percentage between all functions of both profiles.
|
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.