feat(alerts): on demand metric timeseries support #53053
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #53053 +/- ##
==========================================
- Coverage 79.52% 77.80% -1.73%
==========================================
Files 4939 4938 -1
Lines 208076 208088 +12
Branches 35477 35498 +21
==========================================
- Hits 165481 161899 -3582
- Misses 37558 41145 +3587
- Partials 5037 5044 +7
|
| @@ -250,6 +252,10 @@ def _get_aggregate_fields(aggregate: str) -> Sequence[str]: | |||
| return [] | |||
|
|
|||
|
|
|||
| def is_standard_metrics_query(query: Optional[str] = "") -> bool: | |||
There was a problem hiding this comment.
Could we expose just one out of is_on_demand_query and this? This is a misleading function for a public interface, because in order to check for a compatible query you always have to check for both the selected aggregates AND the query (due to countIf).
| # On-demand metrics are implicitly transaction metrics. Remove the | ||
| # filters from the query that can't be translated to a RuleCondition. | ||
| query = re.sub(r"event\.type:transaction\s*", "", query) | ||
| query = re.sub(r"project:\w+\s*", "", query) |
There was a problem hiding this comment.
Yes, handled with 2ea5a82
| assert query.on_demand_metrics_enabled | ||
| assert not query._on_demand_spec |
There was a problem hiding this comment.
This tests class internals, is there some public effect we could observe instead? Such as a call to a (mocked) utility or a flag returned?
|
|
||
| meta = result["meta"] | ||
|
|
||
| assert len(meta) == 2 |
There was a problem hiding this comment.
There should be an is_metrics_data flag in there, can you assert this and check if the current way it's set makes sense?
There was a problem hiding this comment.
Unfortunately, that flag is always set by the callers of query builders, after they receive results such as in
The result itself here contains only "meta" and "data"
obostjancic
force-pushed
the
ogi/feat/on-demand-alerts-chart
branch
from
b9108f0
to
c429213
Compare
| spec = self._on_demand_spec | ||
|
|
||
| # TimeseriesQueryBuilder specific parameters | ||
| if self.is_timeseries_query: |
There was a problem hiding this comment.
Wouldn't it be easier to just check if isinstance(self, TimeSeriesQueryBuilder)
There was a problem hiding this comment.
Yes! Done in 25d8adb
Suspect IssuesThis pull request has been deployed and Sentry has observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Followup of reverted #52867
Part of Support chart queries for on-demand metrics#52508