fix(analytics): catch InfluxDBError in influx_query_manager (fixes #8510) - #8577
siddiqueirshad wants to merge 1 commit into
Conversation
|
@siddiqueirshad is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthrough
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~8 minutes Severity of issue fixed: Medium Merge Risk: 🟠 High · up to InfluxDB HTTP failures can still reach analytics endpoints as 500 responses; handle ApiException before merging. ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 23414984-53c8-47ad-b70b-954fa7ff3e49
📒 Files selected for processing (2)
api/app_analytics/influxdb_wrapper.pyapi/tests/unit/app_analytics/test_unit_app_analytics_influxdb_wrapper.py
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| try: | ||
| return query_api.query(org=settings.INFLUXDB_ORG, query=query) | ||
| except HTTPError as e: | ||
| except (HTTPError, InfluxDBError) as e: | ||
| capture_exception(e) | ||
| return [] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,220p' api/app_analytics/influxdb_wrapper.py | cat -n
rg -n -C 3 'influx_query_manager|InfluxDBWrapper' api --glob '*.py'
sed -n '1,160p' api/tests/unit/app_analytics/test_unit_app_analytics_influxdb_wrapper.py | cat -n
git diff --unified=12 49d957aa3228d8733b43951c099840577ff78338 5ce97097baba97504ae0e062cbfd3ab7ffe88d00 -- api/app_analytics/influxdb_wrapper.py api/tests/unit/app_analytics/test_unit_app_analytics_influxdb_wrapper.py
rg -n -C 2 'influxdb-client|influxdb_client' api/pyproject.toml api/uv.lock | head -50Repository: Flagsmith/flagsmith
Length of output: 41159
🏁 Script executed:
sed -n '60,110p' api/app_analytics/services.py | cat -n
rg -n -C 4 'get_feature_usage|get_multiple_event_list_for_organisation|get_current_api_usage|get_top_organisations|get_usage_data|get_feature_evaluation_data' api --glob '*.py' | head -240Repository: Flagsmith/flagsmith
Length of output: 24800
Handle ApiException in the query handler.
influxdb-client 1.50.0 raises influxdb_client.rest.ApiException for HTTP response failures. The handler catches only HTTPError and InfluxDBError, so 400, 401, 429, and 5xx failures can escape InfluxDBWrapper.influx_query_manager instead of returning []. Analytics callers can therefore fail instead of receiving the existing empty-result fallback.
Add ApiException to the handler and make the added test raise ApiException. Keep InfluxDBError if this boundary must also handle that separate exception type.
Suggested fix
from influxdb_client.client.exceptions import InfluxDBError
from influxdb_client.client.flux_table import FluxTable
from influxdb_client.client.write_api import SYNCHRONOUS
+from influxdb_client.rest import ApiException
...
- except (HTTPError, InfluxDBError) as e:
+ except (HTTPError, InfluxDBError, ApiException) as e:- expected_exception = InfluxDBError(message="InfluxDB error occurred")
+ expected_exception = ApiException(status=400, reason="Bad Request")
Fixes #8510
Cause
influx_query_managercaught onlyurllib3.exceptions.HTTPError, which covers network/socket connection errors but not Influx HTTP error responses (influxdb_client.client.exceptions.InfluxDBError/ApiException). As a result, non-200 responses from InfluxDB propagated uncaught exceptions and resulted in 500 responses on caller endpoints.Solution
(HTTPError, InfluxDBError)ininflux_query_manager(matchingwrite()).influx_query_managerhandlesInfluxDBErrorby capturing the exception and returning an empty list.