sentry icon indicating copy to clipboard operation
sentry copied to clipboard

feat(alerts): Skip metrics_layer when creating a PerformanceMetricsEntitySubscription using insights related aggregates/queries

Open edwardgou-sentry opened this issue 1 year ago • 3 comments

For context, we want to enable alerting on metrics from the Insights modules. However, there is a blocker that this PR attempts to resolve:

Most metrics displayed in the Insights modules are based on the Discover SpansMetricsQueryBuilder without using the Metrics Layer, while Alerts uses AlertMetricsQueryBuilder with the Metrics Layer.

To support Insights metrics in Alerts, a possible solution involves the following two changes:

  1. Port over any fields and functions from the SpansMetricsQueryBuilder config to the AlertMetricsQueryBuilder config
  2. Disable using the Metrics Layer on the AlertMetricsQueryBuilder when the alert is based on Insights metrics. (We cannot use the Metrics Layer because many of the Insights fields and functions cannot be reproduced with the Metrics Layer's capabilities)

This PR involves point 2 from the above. To detect whether an Alert query is Insights based, parse the aggregate and query. We do it this way because there are no other attributes on the Alert Rule that we can leverage to directly tell us if an Alert is based on an Insight metric (technically they're just generic metrics).

edwardgou-sentry avatar Aug 07 '24 20:08 edwardgou-sentry

Codecov Report

Attention: Patch coverage is 89.18919% with 4 lines in your changes missing coverage. Please review.

Project coverage is 78.26%. Comparing base (1eccd67) to head (5279b83). Report is 213 commits behind head on master.

:white_check_mark: All tests successful. No failed tests found.

Files Patch % Lines
src/sentry/search/events/builder/metrics.py 87.50% 2 Missing and 2 partials :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #75765      +/-   ##
==========================================
+ Coverage   78.25%   78.26%   +0.01%     
==========================================
  Files        6833     6820      -13     
  Lines      303569   303808     +239     
  Branches    52248    52252       +4     
==========================================
+ Hits       237546   237776     +230     
  Misses      59643    59643              
- Partials     6380     6389       +9     
Files Coverage Δ
src/sentry/search/events/constants.py 100.00% <100.00%> (ø)
src/sentry/search/events/types.py 91.25% <100.00%> (+0.22%) :arrow_up:
src/sentry/snuba/entity_subscription.py 90.51% <ø> (ø)
src/sentry/search/events/builder/metrics.py 88.54% <87.50%> (-0.08%) :arrow_down:

... and 181 files with indirect coverage changes

codecov[bot] avatar Aug 07 '24 21:08 codecov[bot]

As discussed in slack, lets try to use the query parsing in the builder to make this determination instead of using regexes on the raw query and aggregate strings

Updated! I moved the logic into MetricsQueryBuilder and added a insights_metrics_override_metric_layer builder config option to control this

edwardgou-sentry avatar Aug 08 '24 20:08 edwardgou-sentry

Fixed some issues with use_case_id not using SPAN when appropriate

edwardgou-sentry avatar Aug 09 '24 18:08 edwardgou-sentry