nit: include query_extra for metric alert notifications #73325
Conversation
github-actions
bot
added
Scope: Frontend
|
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
| @@ -162,8 +163,7 @@ def build_metric_alert_chart( | |||
| end: str | None = None, | |||
| user: Optional["User"] = None, | |||
| size: ChartSize | None = None, | |||
| query_extra: str | None = None, | |||
| project_id: int | None = None, | |||
| subscription: QuerySubscription | None = None, | |||
There was a problem hiding this comment.
rather than passing query_extra here and relying on implementations to construct the query_extra, instead we accept the subscription and construct the query_extra within the chart builder.
| @@ -217,6 +221,8 @@ def metric_alert_attachment_info( | |||
| if latest_incident: | |||
| last_triggered_date = latest_incident.date_started | |||
|
|
|||
| activation = selected_incident.activation | |||
| # TODO: determine whether activated alert data is useful for integration messages | |||
There was a problem hiding this comment.
preemptively include activated alert data into the metric alert attachment info
nhsiehgit
changed the title
| @@ -77,6 +77,7 @@ function MetricAlertActivity({organization, incident}: MetricAlertActivityProps) | |||
| }/releases/${encodeURIComponent(activation.activator)}/`, | |||
| query: {project: project}, | |||
| }} | |||
| style={{textOverflow: 'ellipsis', overflowX: 'inherit'}} | |||
There was a problem hiding this comment.
oops. this is an unrelated change to make the alert row slightly nicer.
shouldn't have any effect on the rest of the PR...
I can break this out if anyone feels strongly
thanks for doing this, i feel like this is a really great cleanup pr and does a great job of DRYing up the code.
src/sentry/snuba/utils.py
Outdated
| @@ -32,3 +33,13 @@ | |||
|
|
|||
| def get_dataset(dataset_label: str) -> Any | None: | |||
| return DATASET_OPTIONS.get(dataset_label) | |||
|
|
|||
|
|
|||
| def build_query_extra(subscription: QuerySubscription | None, snuba_query: SnubaQuery) -> str: | |||
There was a problem hiding this comment.
nit: might be nice to add some tests to this as we break it out.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #73325 +/- ##
==========================================
- Coverage 78.03% 78.01% -0.02%
==========================================
Files 6632 6632
Lines 296035 296117 +82
Branches 50994 50997 +3
==========================================
+ Hits 231002 231024 +22
- Misses 58750 58806 +56
- Partials 6283 6287 +4
|
we're constructing
query_extrain a few spots now.This consolidates the query_extra string all to utilize a single method to derive the query extra from the existing snuba_query and subscription
NOTE:
There are a handful of areas where we are referencing
snuba_query.querywithout taking into account thequery_extraThis might possibly lead to discrepancies if we are updating the snuba_query
query, comparing queries, or executing one off snuba queries 🤔eg.
etc.etc.
TODO: we may want to audit all uses and determine whether we should be constructing the query from the subscription rather than the
snuba_query🤔