fix: describe the anomalous bucket in non-dimension anomaly alerts - #1043
fix: describe the anomalous bucket in non-dimension anomaly alerts#1043joostboon wants to merge 2 commits into
Conversation
|
👋 @joostboon |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe anomaly result description selects the latest relevant bucket. An integration test verifies that a volume anomaly reports the anomalous bucket’s row count. ChangesVolume anomaly description
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The non-dimension branch of the anomaly test description picked rows_with_score[-1]. That has two problems: 1. rows_with_score holds every bucket with a non-null anomaly score, not just the anomalous ones, so the description could describe a bucket that passed. 2. The anomaly scores query has no "order by", so [-1] is whichever row the warehouse happened to return last, not the latest bucket. Observed on a volume_anomalies test where the only anomalous bucket was a spike to 6 (training avg 1.095), but the alert read "The last row_count value is 0.000. The average for this metric is 1.238." Those numbers belonged to a bucket two days earlier with anomaly_score -1.05 and is_anomalous false. The alert inverted a spike into a drop to zero. Prefer anomalous_rows when the test failed, fall back to rows_with_score when it passed, and sort by bucket_end in both cases. This mirrors what #1032 already did for the dimension branch.
32a2ebf to
2fdfe35
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
integration_tests/tests/test_volume_anomalies.py (1)
50-73: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover ordering between multiple anomalous buckets.
This fixture creates only one anomalous bucket, so it does not exercise
sort(attribute='bucket_end') | last. Add a second anomalous bucket and assert that the description reports the later bucket. Otherwise, a regression that selects an arbitrary anomalous row can pass this test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration_tests/tests/test_volume_anomalies.py` around lines 50 - 73, Update test_volume_anomalies_description_reports_anomalous_bucket to include a second anomalous bucket with a later date and assert that test_results_description reports the later bucket’s value. Keep the existing training data and failure assertion, and ensure the assertion distinguishes the later anomaly from the earlier one so bucket ordering and selection are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@integration_tests/tests/test_volume_anomalies.py`:
- Around line 50-73: Update
test_volume_anomalies_description_reports_anomalous_bucket to include a second
anomalous bucket with a later date and assert that test_results_description
reports the later bucket’s value. Keep the existing training data and failure
assertion, and ensure the assertion distinguishes the later anomaly from the
earlier one so bucket ordering and selection are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c64830da-dd9e-46d3-9a97-7701eeea441b
📒 Files selected for processing (2)
integration_tests/tests/test_volume_anomalies.pymacros/edr/data_monitoring/anomaly_detection/store_anomaly_test_results.sql
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
ClickHouse and BigQuery render the rounded metric value as "6" where Snowflake renders "6.000", so the literal substring assertion failed on those adapters even though the description named the correct bucket.
1 participant
Problem tl;dr
In our description, we pick the bucket that was last non anomalous. This is actually wrong behaviour. We should take the anomalous value (see screenshot). It says "The last row_count value is 0.000.", but it should say in this case "The last row_count value is 6.000."
More info
The non-dimension branch of the anomaly test description picks
rows_with_score[-1]:https://github.com/elementary-data/dbt-data-reliability/blob/master/macros/edr/data_monitoring/anomaly_detection/store_anomaly_test_results.sql#L114
Two defects stack on each other:
rows_with_scoreis every bucket with a non-nullanomaly_score, not the anomalous ones. The macro already buildsanomalous_rowsa few lines above and uses it in the dimension branch, but the{% else %}branch ignores it, so the description can describe a bucket that passed.[-1]on an unordered result. The anomaly scores query has noorder by, so[-1]is whichever row the warehouse returned last, not the latest bucket. The comment on the dimension branch already notes this.Observed impact
On a
volume_anomaliestest the execution returned 140 scored buckets with exactly oneis_anomalous = true:metric_valuetraining_avganomaly_scoreanomaly_descriptionThe alert that was rendered and sent to Slack said instead:
Those numbers belong to the 2026-08-03 → 2026-08-04 bucket, which had
anomaly_score = -1.05andis_anomalous = false. It was the only row in the set withtraining_avg = 1.238, so the source is unambiguous.The practical consequence is that the alert inverted the story: a spike was reported as a drop to zero. Anyone triaging from Slack goes looking for a broken ingestion pipeline that isn't broken.
Fix
Prefer
anomalous_rowswhen the test failed, fall back torows_with_scorewhen it passed, and sort bybucket_endin both cases so[-1]means "latest bucket". This mirrors what #1032 already did for the dimension branch, which fixed the same class of bug but only inside{% if dimension %}.Testing
test_volume_anomalies_description_reports_anomalous_bucketcovering the reported shape: single-row training buckets, detected bucket spikes to six, description must report the spike.pre-commit runpasses on both changed files (black, isort, flake8, prettier, typos, sqlfmt).Existing description assertions live in
test_column_anomalies.pyand all target the dimension branch or the "not enough data" path, so nothing asserted the old unsorted behavior.Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Tests