-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(preprod): add endpoint to fetch dashboard data #105309
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
base: master
Are you sure you want to change the base?
Conversation
|
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
src/sentry/preprod/api/endpoints/organization_preprod_app_size_stats.py
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #105309 +/- ##
===========================================
- Coverage 80.56% 80.54% -0.02%
===========================================
Files 9443 9444 +1
Lines 404657 404926 +269
Branches 25722 25722
===========================================
+ Hits 326002 326160 +158
- Misses 78223 78334 +111
Partials 432 432 |
src/sentry/preprod/api/endpoints/organization_preprod_app_size_stats.py
Outdated
Show resolved
Hide resolved
src/sentry/preprod/api/endpoints/organization_preprod_app_size_stats.py
Outdated
Show resolved
Hide resolved
src/sentry/preprod/api/endpoints/organization_preprod_app_size_stats.py
Outdated
Show resolved
Hide resolved
| include_filters = request.GET.get("includeFilters", "").lower() == "true" | ||
| if include_filters: | ||
| filter_values = self._fetch_filter_values(organization.id, project_ids, start, end) | ||
| result["filters"] = filter_values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: The optional filter query, triggered by includeFilters=true, lacks error handling. A failure in the underlying Snuba RPC call will crash the entire endpoint.
Severity: HIGH | Confidence: High
🔍 Detailed Analysis
The endpoint handles errors for the main data query but not for the optional filter data query triggered when includeFilters=true. The _fetch_filter_values method calls query_preprod_size_metrics without a try-except block. Since query_preprod_size_metrics can raise several exceptions (e.g., SnubaRPCError), a failure in this optional part of the request will cause the entire endpoint to return a 500 error, even if the main data was successfully retrieved. This violates the principle that optional enhancements should not crash core functionality.
💡 Suggested Fix
Wrap the call to self._fetch_filter_values within a try-except block. In the except block, log the error and gracefully degrade by returning the main data without the filter values, or by setting filter_values to an empty dictionary. This ensures that a failure in the optional filter query does not impact the primary functionality of the endpoint.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
src/sentry/preprod/api/endpoints/organization_preprod_app_size_stats.py#L117-L120
Potential issue: The endpoint handles errors for the main data query but not for the
optional filter data query triggered when `includeFilters=true`. The
`_fetch_filter_values` method calls `query_preprod_size_metrics` without a `try-except`
block. Since `query_preprod_size_metrics` can raise several exceptions (e.g.,
`SnubaRPCError`), a failure in this optional part of the request will cause the entire
endpoint to return a 500 error, even if the main data was successfully retrieved. This
violates the principle that optional enhancements should not crash core functionality.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7771806
Adds a new endpoint we can query from Dashboards to fetch preprod EAP data.