-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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(insights): Update backend performance score function to handle missing vitals #82750
feat(insights): Update backend performance score function to handle missing vitals #82750
Conversation
…ls, instead of just considering them to be zero
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #82750 +/- ##
==========================================
+ Coverage 87.46% 87.66% +0.20%
==========================================
Files 9407 9410 +3
Lines 536317 545069 +8752
Branches 21044 20977 -67
==========================================
+ Hits 469088 477835 +8747
- Misses 66859 66863 +4
- Partials 370 371 +1 |
# TODO: Is there a way to sum more than 2 values at once? | ||
return Function( | ||
"plus", | ||
"divide", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We divide by the maximum possible weight (which will be some value between 0 - 1.0 depending on which vitals are present). This will scale up the result if there are any vitals missing.
"if", | ||
[ | ||
Function( | ||
"isZeroOrNull", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both a 0 or a null value represent a missing vital score?
This makes sense to me, because you can't ever have an instantaneous load time for any vital metric.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The isZeroOrNull
condition is ran on the count
of the metric so we're really just checking for the existence of the vital, not the actual value. This is false if an lcp
with value of 0ms
is reported, because the count is > 0 in this case.
You're also right, it shouldn't really be possible for load time vitals to be 0ms. It might be possible for cls to be 0 though, and that would depend on if the sdk reports it.
Not sure if there's an easier clickhouse or snuba function to do this check, but this works. We'll see if there are any suggestions
Refs: #77071 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code makes sense! Could you add a small explanation to the function, though? The main thing that confused me is that there are weight columns in the actual metric buckets rows in ClickHouse and now there are weights when calculating the score. What's the difference between the weights?
Added an explanation! |
Currently, if a webvital is missing, we treat the individual score as 0. This can be misleading because it negatively affects the overall performance score of an organization/project.
Instead, we want to remove any missing vital factors from the overall performance score calculation, which is what this change aims to do by updating the
performance_score
function.