feat(alerts): Add support for trace metrics alert type #104901
Conversation
Add TRACE_ITEM_METRIC to valid event types for performance alerts. Update entity subscription to use TraceMetrics module for trace metric alerts.
github-actions
bot
added
the
Scope: Backend
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return trace_metric | ||
|
|
||
| except (InvalidSearchQuery, Exception) as e: | ||
| raise serializers.ValidationError({"aggregate": f"Invalid trace metrics aggregate: {e}"}) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 27 days ago
To fix the problem, we should ensure that exception messages containing potentially sensitive information are not exposed to users. Instead, we should log the exception details for server-side debugging and return a generic message in the validation error. This requires:
- Catching and logging the full exception details internally (using Python's logging facilities),
- Returning a user-safe generic error message (e.g., "Invalid trace metrics aggregate"), rather than directly exposing
str(e).
Specifically, invalidate_trace_metrics_aggregate, modify theexceptblock to log the exception and use a generic error message in the response.
Additionally, ensure that the logging import (import logging) is in the file and a logger is instantiated (logger = logging.getLogger(__name__)).
All changes are withinsrc/sentry/search/eap/trace_metrics/validator.py.
| @@ -1,7 +1,9 @@ | ||
| from datetime import datetime, timedelta, timezone | ||
|
|
||
| from rest_framework import serializers | ||
| import logging | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
| from sentry.exceptions import InvalidSearchQuery | ||
| from sentry.search.eap.resolver import SearchResolver | ||
| from sentry.search.eap.trace_metrics.config import TraceMetricsSearchResolverConfig | ||
| @@ -63,4 +64,5 @@ | ||
| return trace_metric | ||
|
|
||
| except (InvalidSearchQuery, Exception) as e: | ||
| raise serializers.ValidationError({"aggregate": f"Invalid trace metrics aggregate: {e}"}) | ||
| logger.exception("Error validating trace metrics aggregate: %s", aggregate) | ||
| raise serializers.ValidationError({"aggregate": "Invalid trace metrics aggregate"}) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #104901 +/- ##
===========================================
- Coverage 80.64% 80.56% -0.08%
===========================================
Files 9335 9343 +8
Lines 403011 402073 -938
Branches 25695 25695
===========================================
- Hits 325001 323944 -1057
- Misses 77544 77663 +119
Partials 466 466 |
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you add the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| status_code=404, | ||
| **data, | ||
| ) | ||
| assert "The requested resource does not exist" in str(resp.data["detail"]) |
There was a problem hiding this comment.
Test passes for wrong reason due to missing feature flag
The test test_create_alert_rule_trace_metrics_feature_flag_disabled claims to test trace metrics feature flag behavior but is missing organizations:incidents in its feature flags. The endpoint checks for incidents first and returns 404 if disabled, so this test gets a 404 from the incidents check rather than the trace metrics check. The test would still pass even if the trace metrics feature flag check was accidentally removed from production code, providing false confidence in the feature flag validation.
Add TRACE_ITEM_METRIC to valid event types for performance alerts. Update entity subscription to use TraceMetrics module for trace metric alerts.
Additionally, this adds more thorough validation for EAP queries, specifically for
trace_metricsas they have a unique aggregate syntax (instead ofcount(column)it's almost alwayscount(column, <metric tuple>)) that we need to enforce. This should work on both sides of old and new alerts so we're forwards and backwards compatible during the workflow switch over.