feat(generic-metrics): Add dropped percentiles to aggregation options #70824
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #70824 +/- ##
==========================================
+ Coverage 77.87% 77.92% +0.04%
==========================================
Files 6529 6525 -4
Lines 290909 290480 -429
Branches 50338 50254 -84
==========================================
- Hits 226548 226348 -200
+ Misses 58122 57874 -248
- Partials 6239 6258 +19
|
| } | ||
|
|
||
|
|
||
| def get_aggregation_options(mri: str, org_id: int) -> dict[AggregationOption, TimeWindow] | None: |
There was a problem hiding this comment.
Since we are only able to write 1 aggregation option at the moment, what if we added a test case which validated that assumption. I am not exactly sure whether that constraint is enforced somewhere in the code or not. But it would be good to have the constrain in place to avoid potential problems in the near future.
There was a problem hiding this comment.
Good point. Added some logic to our existing unit tests where there are a variety of aggregation options set up, which cover all the use cases etc. The unit test asserts that only 1 exists per metric bucket payload, though.
Once we have multiple aggregation option support, we can remove and refactor these tests to communicate that change in expectations.
What do you think of these changes?
There was a problem hiding this comment.
What if we change the signature of this function to -> (AggregationOption, TimeWindow) | None to enforce this in application code? Once we support multiple options, we can easily change the signature again.
There was a problem hiding this comment.
The indexer already assumes a dict output from calling get_aggregation_options and basically chooses the first element. So in any case, we are always guaranteed to have 1 aggregation option per payload.
I can change the typing and indexer behavior in a subsequent PR, but for now I think I'd like to not have this PR touch too many files at once.
I added some unit tests which should hopefully make the behavior/sequence of operations in get_aggregation_options clearer.
| } | ||
|
|
||
|
|
||
| def get_aggregation_options(mri: str, org_id: int) -> dict[AggregationOption, TimeWindow] | None: |
There was a problem hiding this comment.
What if we change the signature of this function to -> (AggregationOption, TimeWindow) | None to enforce this in application code? Once we support multiple options, we can easily change the signature again.
|
|
||
| # Set various aggregation options that | ||
| # are use case-wide aggregations | ||
| set_use_case_aggregation_options() |
There was a problem hiding this comment.
This function internally modifies a global, but this is done on every call of get_aggregation_options. The global isn't really needed therefore, it could be just a local variable. This would further allow you to simplify and inline the checks into a single if-chain below that would also more clearly show precedence of the options.
|
Will add more unit tests to check that the logic in |
| return USE_CASE_AGG_OPTION[use_case_id] | ||
| elif use_case_id in use_case_agg_options: | ||
| if org_id not in drop_uc_org_override.get(use_case_id.value, []): | ||
| return use_case_agg_options[use_case_id] |
There was a problem hiding this comment.
As an optional suggestion, this could be easier to read and follow without use_case_agg_options. This branch could check directly the options and then early return the aggregation option.
Pseudo code skeleton for the entire function:
if org_id in options.get("per-org", []):
return DISABLE_PERCENTILES
opt = options.get("with-override", {})
if use_case in opt and org_id not in opt[use_case]:
return DISABLE_PERCENTILES
if mri in METRIC_ID_AGG_OPTION:
return METRIC_ID_AGG_OPTION[mri]
if use_case == CUSTOM and options.get("10s"):
return TEN_SECOND
return {}
src/sentry/options/defaults.py
Outdated
| @@ -1260,6 +1260,15 @@ | |||
| flags=FLAG_AUTOMATOR_MODIFIABLE, | |||
| ) | |||
|
|
|||
| # Option to remove support for percentiles on a per-use case basis. | |||
| # Add the use case to list to disable percentiles. | |||
There was a problem hiding this comment.
nit: This isn't a list, the use case must be an entry with a list value (otherwise causing a crash). Should we make this more clear?
Refactoring the way we used an older option for enabling 10s granularity so its purpose is clearer.
Enable org-level and use case-level of disabling percentiles using the newly created Sentry options.