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

fix(anomaly detection): get aggregation key from snuba data #77498

Merged
merged 2 commits into from
Sep 13, 2024

Conversation

mifu67
Copy link
Contributor

@mifu67 mifu67 commented Sep 13, 2024

We can't assume that the aggregation key of a snuba timeseries data point will be "count", so we should set the aggregation key when we see a data point with an aggregation value.

@mifu67 mifu67 requested a review from ceorourke September 13, 2024 18:55
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 13, 2024
Copy link

codecov bot commented Sep 13, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/sentry/seer/anomaly_detection/utils.py 88.88% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #77498       +/-   ##
===========================================
+ Coverage   57.38%   78.13%   +20.74%     
===========================================
  Files        6945     6955       +10     
  Lines      308220   308773      +553     
  Branches    50449    50541       +92     
===========================================
+ Hits       176883   241270    +64387     
+ Misses     126537    61107    -65430     
- Partials     4800     6396     +1596     

@@ -102,8 +102,21 @@ def format_historical_data(data: SnubaTSResult, dataset: Any) -> list[TimeSeries
ts_point = TimeSeriesPoint(timestamp=date.timestamp(), value=count)
formatted_data.append(ts_point)
else:
# we don't know what the aggregation key of the query is
# so we should see it when we see a data point that has a value
agg_key = ""
Copy link
Member

Choose a reason for hiding this comment

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

I think this looks good, but can we add some tests?

@mifu67 mifu67 merged commit 2de49ac into master Sep 13, 2024
48 of 49 checks passed
@mifu67 mifu67 deleted the mifu67/anomaly-detection-bugs-4 branch September 13, 2024 22:31
@github-actions github-actions bot locked and limited conversation to collaborators Sep 29, 2024
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants