-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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): Add division_if for span metrics #72995
Conversation
Add division_if for span metrics so we can calculate rates such as slow and frozen rates.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #72995 +/- ##
===========================================
+ Coverage 64.40% 78.08% +13.68%
===========================================
Files 6673 6677 +4
Lines 298641 298874 +233
Branches 51437 51466 +29
===========================================
+ Hits 192350 233389 +41039
+ Misses 99806 59219 -40587
+ Partials 6485 6266 -219
|
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.
Just a comment about the test setup, but I think the code looks good
self.store_span_metric( | ||
1, | ||
internal_metric=constants.SPAN_METRICS_MAP["mobile.slow_frames"], | ||
timestamp=self.three_days_ago, | ||
tags={"release": "1.0.0"}, | ||
) |
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.
These look like they'd pass but that's just because of how the data is being mocked and where the mock data is set up. There's another example of slow frames tests here where the first arg is actually passed as an object which is consistent for the gauge type, I'd suggest changing these to match that type to make sure it's working the way you want for these metrics, e.g.
self.store_span_metric(
{
"min": 1,
"max": 1,
"sum": 1,
"count": 1,
"last": 1,
},
entity="metrics_gauges",
metric="mobile.slow_frames",
timestamp=self.six_min_ago,
tags={"release": "foo"},
)
Total frames might be the only one that needs to have different values for min/max/sum/count/last when entered
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you add the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
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.
Looks good, just left a comment on naming. Thanks for finding that column
is actually a special arg that we need to define, we won't be able to change that one but we could do the second column
Add division_if for span metrics so we can calculate rates such as slow and frozen rates. --------- Co-authored-by: Markus Hintersteiner <markus.hintersteiner@sentry.io>
As described [here](#73039), we want to move to ratios for slow and frozen frames. Utilizes [division_if](#72995) to calculate the ratios. <img width="1240" alt="image" src="https://github.com/user-attachments/assets/66d0b3ee-22ff-43eb-aee6-9242b9d3404a">
Add division_if for span metrics so we can calculate rates such as slow and frozen rates.