fix: include all failing dimension values in anomaly alert description#1032
Open
joostboon wants to merge 2 commits into
Open
fix: include all failing dimension values in anomaly alert description#1032joostboon wants to merge 2 commits into
joostboon wants to merge 2 commits into
Conversation
Previously, dimension-based anomaly tests only showed a single (arbitrary) dimension value in the alert description. Now the description lists all failing dimension values with their metric and average values, truncated to 5 with an "and N more" suffix when there are many failures. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…by tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
👋 @joostboon |
📝 WalkthroughWalkthroughThe SQL macro ChangesDimension-aware anomaly description
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
integration_tests/tests/test_column_anomalies.py (1)
320-323: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one case that exercises the
max_shown/"and N more"branch.These assertions only cover 1- and 2-dimension failures, but the new formatter has separate behavior once there are more than 5 anomalous dimensions. A >5 case would keep the truncation and suffix logic from regressing silently.
Also applies to: 343-347
🤖 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_column_anomalies.py` around lines 320 - 323, The current assertions in the anomaly description tests only cover the small-dimension cases, so add a new test case in test_column_anomalies that drives the formatter past the max_shown threshold and verifies the “and N more” suffix. Use the existing description checks around test_result["test_results_description"] to assert the truncated output and the extra-count branch, ensuring the formatter logic in the test results description path does not regress.
🤖 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.
Inline comments:
In `@macros/edr/data_monitoring/anomaly_detection/store_anomaly_test_results.sql`:
- Around line 79-107: The summary in store_anomaly_test_results.sql is counting
and listing duplicate dimension_value entries from anomalous_rows, which can
overstate failures for a single dimension. Update the test_results_description
construction to deduplicate anomalous_rows by dimension_value before generating
dim_parts and total, keeping the latest or highest-priority row per value. Then
apply the existing max_shown cap to the unique list so the reported count and
examples reflect distinct failing dimensions.
---
Nitpick comments:
In `@integration_tests/tests/test_column_anomalies.py`:
- Around line 320-323: The current assertions in the anomaly description tests
only cover the small-dimension cases, so add a new test case in
test_column_anomalies that drives the formatter past the max_shown threshold and
verifies the “and N more” suffix. Use the existing description checks around
test_result["test_results_description"] to assert the truncated output and the
extra-count branch, ensuring the formatter logic in the test results description
path does not regress.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64f4da88-9811-415a-88a8-6e8bd30533e4
📒 Files selected for processing (2)
integration_tests/tests/test_column_anomalies.pymacros/edr/data_monitoring/anomaly_detection/store_anomaly_test_results.sql
Comment on lines
+79
to
+107
| {% set anomalous_rows = [] %} | ||
| {% for row in anomaly_scores_rows %} | ||
| {% if row.anomaly_score is not none %} | ||
| {% do filtered_anomaly_scores_rows.append(row) %} | ||
| {% if row.is_anomalous %} | ||
| {% set failures.data = failures.data + 1 %} | ||
| {% do anomalous_rows.append(row) %} | ||
| {% endif %} | ||
| {% endif %} | ||
| {% endfor %} | ||
| {% set test_results_description %} | ||
| {% if rows_with_score %} | ||
| {% set first_scored_row = rows_with_score[0] %} | ||
| {% set dimension = elementary.insensitive_get_dict_value(first_scored_row, 'dimension') %} | ||
| {% if dimension and anomalous_rows %} | ||
| {% set max_shown = 5 %} | ||
| {% set shown_rows = anomalous_rows[:max_shown] %} | ||
| {% set dim_parts = [] %} | ||
| {% for row in shown_rows %} | ||
| {% set dim_val = elementary.insensitive_get_dict_value(row, 'dimension_value') %} | ||
| {% set dim_val_str = dim_val if dim_val is not none else 'NULL' %} | ||
| {% set m_val = elementary.insensitive_get_dict_value(row, 'metric_value') %} | ||
| {% set t_val = elementary.insensitive_get_dict_value(row, 'training_avg') %} | ||
| {% set m_str = (m_val | float | round(3)) if m_val is not none else 'N/A' %} | ||
| {% set t_str = (t_val | float | round(3)) if t_val is not none else 'N/A' %} | ||
| {% do dim_parts.append(dim_val_str ~ " (" ~ m_str ~ ", avg " ~ t_str ~ ")") %} | ||
| {% endfor %} | ||
| {% set total = anomalous_rows | length %} | ||
| {% if column_name %}In column {{ column_name | upper }}, {% endif %}{{ total }} anomalous {{ metric_name }} value{% if total > 1 %}s{% endif %} for dimension {{ dimension }}: {{ dim_parts | join(", ") }}{% if total > max_shown %}, and {{ total - max_shown }} more{% endif %}. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Deduplicate dimension_value before building the summary.
anomalous_rows collects every anomalous scored row in the (full_table_name, column_name, metric_name) group. Because those rows are bucket-based, the same dimension can appear multiple times here, so total and dim_parts can repeat a single failing dimension and overstate the “N anomalous … values” count. Build the description from unique dimension_values first (keeping the latest or highest-priority row per value), then calculate total and apply the 5-item cap.
🤖 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 `@macros/edr/data_monitoring/anomaly_detection/store_anomaly_test_results.sql`
around lines 79 - 107, The summary in store_anomaly_test_results.sql is counting
and listing duplicate dimension_value entries from anomalous_rows, which can
overstate failures for a single dimension. Update the test_results_description
construction to deduplicate anomalous_rows by dimension_value before generating
dim_parts and total, keeping the latest or highest-priority row per value. Then
apply the existing max_shown cap to the unique list so the reported count and
examples reflect distinct failing dimensions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
column_anomalieswithdimensions) previously showed only a single arbitrary dimension value in the result messageIn column INCIDENTS_OPENED, 2 anomalous max values for dimension account_name: Joejuice (214.0, avg 21.0), Acme (15.0, avg 6.3).Test plan
test_column_anomalies_group_byto verify the description format for 1 and 2 failing dimension valuescolumn_anomaliestest withdimensions-- alert now shows all failing dimension valuesbefore and after screenshots:
