Skip to content
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(alerts): on demand metric time series support #52867

Merged
merged 11 commits into from
Jul 18, 2023

Conversation

obostjancic
Copy link
Member

@obostjancic obostjancic commented Jul 14, 2023

Related to Support chart queries for on-demand metrics#52508

Enables time series querying of on demand metrics

@github-actions github-actions bot added Scope: Frontend Automatically applied to PRs that change frontend components Scope: Backend Automatically applied to PRs that change backend components labels Jul 14, 2023
@@ -101,7 +101,7 @@ def _resolve_on_demand_spec(
return None

field = selected_cols[0] if selected_cols else None
if not self.is_alerts_query or not field:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need this to execute also for TimeseriesQueryBuilder now

@obostjancic obostjancic force-pushed the ogi/feat/on-demand-alerts-chart branch from b78d567 to c4ecde8 Compare July 17, 2023 06:59
@obostjancic obostjancic removed the Scope: Frontend Automatically applied to PRs that change frontend components label Jul 17, 2023
@codecov
Copy link

codecov bot commented Jul 17, 2023

Codecov Report

Merging #52867 (1eb6e1d) into master (54a1d8f) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 1eb6e1d differs from pull request most recent head 854d407. Consider uploading reports for the commit 854d407 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #52867      +/-   ##
==========================================
- Coverage   79.47%   79.46%   -0.01%     
==========================================
  Files        4936     4936              
  Lines      207488   207492       +4     
  Branches    35423    35444      +21     
==========================================
- Hits       164892   164875      -17     
- Misses      37565    37582      +17     
- Partials     5031     5035       +4     
Impacted Files Coverage Δ
src/sentry/search/events/builder/metrics.py 83.92% <100.00%> (+0.59%) ⬆️
src/sentry/snuba/metrics/extraction.py 88.38% <100.00%> (+1.51%) ⬆️

... and 37 files with indirect coverage changes

@obostjancic obostjancic changed the title feat(alerts): on demand metric alert chart feat(alerts): on demand metric time series support Jul 17, 2023
@getsentry getsentry deleted a comment from github-actions bot Jul 17, 2023
@obostjancic obostjancic marked this pull request as ready for review July 17, 2023 12:29
@obostjancic obostjancic requested a review from a team as a code owner July 17, 2023 12:29
Copy link
Member

@iambriccardo iambriccardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a minor comment

src/sentry/search/events/builder/metrics.py Show resolved Hide resolved
src/sentry/search/events/builder/metrics.py Show resolved Hide resolved
@@ -136,6 +136,7 @@
"os.name",
"geo.country_code",
"event.type",
"project",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a comment before event.type that these are query filters we support as input, but they will not be converted into a rule condition?

I think you still need to account for project somehow in conversion, because it cannot be a valid rule condition. How does this turn up and how are you planning to handle this? Ideally, please add a unit test for this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment
Removed the project field, it was there just for one of the tests

@obostjancic obostjancic enabled auto-merge (squash) July 18, 2023 08:39
@obostjancic obostjancic merged commit f95d541 into master Jul 18, 2023
55 checks passed
@obostjancic obostjancic deleted the ogi/feat/on-demand-alerts-chart branch July 18, 2023 08:57
@obostjancic obostjancic added the Trigger: Revert add to a merged PR to revert it (skips CI) label Jul 18, 2023
@getsentry-bot
Copy link
Contributor

PR reverted: 4856ab5

getsentry-bot added a commit that referenced this pull request Jul 18, 2023
This reverts commit f95d541.

Co-authored-by: obostjancic <86684834+obostjancic@users.noreply.github.com>
@github-actions github-actions bot locked and limited conversation to collaborators Aug 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components Trigger: Revert add to a merged PR to revert it (skips CI)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants