feat(integration): Add health-tracking integration feature#7956
feat(integration): Add health-tracking integration feature#7956afsuyadi wants to merge 50 commits into
Conversation
…ration serializer
afsuyadi
requested review from
emyller and
talissoncosta
and removed request for
a team
|
@afsuyadi is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
for more information, see https://pre-commit.ci
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds an Estimated code review effort: 4 (Complex) | ~60 minutes Comment |
github-actions
Bot
added
front-end
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/integrations/sentry/change_tracking.py (1)
94-107: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftNetwork-level failures never produce a health record.
record_integration_healthis only reached on the success path (line 100), beforeraise_for_status(). Ifrequests.postitself raises (ConnectionError,Timeout, etc.), execution never reaches line 100, so no health record is written at all — the integration's health status silently goes stale instead of reflecting the outage.💡 Suggested direction
try: response = requests.post( url=self.webhook_url, headers=headers, data=json_payload, + timeout=10, ) record_integration_health(self.config, response.status_code) response.raise_for_status() # NOTE: This is for future-proofing, as Sentry won't respond 4xx. except requests.exceptions.RequestException as error: + record_integration_health(self.config, 0) log.warning( "request-failure", error=error, ) return
get_latest_integration_healthalready treats any non-2xxstatus_codeas unhealthy, so a sentinel value like0would correctly surface as "Unhealthy". This same gap likely applies to the other refactored wrappers (e.g.new_relic.py, which has no exception handling at all around its POST call).api/integrations/mixpanel/mixpanel.py (1)
22-40: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard the health write behind best-effort handling.
record_integration_health(...)can raise, andidentify_user_async()does not catch it, so a DB hiccup here can fail the Mixpanel identify call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b1890d82-3475-42d1-a441-d6f46c22fa78
📒 Files selected for processing (44)
api/app/settings/common.pyapi/audit/signals.pyapi/integrations/amplitude/amplitude.pyapi/integrations/amplitude/serializers.pyapi/integrations/common/apps.pyapi/integrations/common/migrations/0001_initial.pyapi/integrations/common/migrations/__init__.pyapi/integrations/common/models.pyapi/integrations/common/serializers.pyapi/integrations/common/services.pyapi/integrations/datadog/datadog.pyapi/integrations/dynatrace/dynatrace.pyapi/integrations/dynatrace/serializers.pyapi/integrations/grafana/grafana.pyapi/integrations/heap/heap.pyapi/integrations/heap/serializers.pyapi/integrations/mixpanel/mixpanel.pyapi/integrations/mixpanel/serializers.pyapi/integrations/new_relic/new_relic.pyapi/integrations/rudderstack/serializers.pyapi/integrations/segment/serializers.pyapi/integrations/sentry/change_tracking.pyapi/integrations/sentry/serializers.pyapi/integrations/webhook/serializers.pyapi/integrations/webhook/webhook.pyapi/tests/integration/sentry/test_change_tracking_webhook_integration.pyapi/tests/unit/integrations/amplitude/test_unit_amplitude.pyapi/tests/unit/integrations/amplitude/test_unit_amplitude_views.pyapi/tests/unit/integrations/common/test_unit_integrations_common_serializers.pyapi/tests/unit/integrations/common/test_unit_integrations_common_services.pyapi/tests/unit/integrations/datadog/test_unit_datadog.pyapi/tests/unit/integrations/dynatrace/test_unit_dynatrace.pyapi/tests/unit/integrations/dynatrace/test_unit_dynatrace_views.pyapi/tests/unit/integrations/grafana/test_grafana.pyapi/tests/unit/integrations/heap/test_unit_heap.pyapi/tests/unit/integrations/heap/test_unit_heap_views.pyapi/tests/unit/integrations/mixpanel/test_unit_mixpanel.pyapi/tests/unit/integrations/mixpanel/test_unit_mixpanel_views.pyapi/tests/unit/integrations/new_relic/test_unit_new_relic.pyapi/tests/unit/integrations/rudderstack/test_unit_rudderstack_views.pyapi/tests/unit/integrations/segment/test_unit_segment_views.pyapi/tests/unit/integrations/webhook/test_unit_webhook.pyfrontend/common/types/responses.tsfrontend/web/components/IntegrationList.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6d8fc160-2c9d-479f-91c1-e8c57287cd64
📒 Files selected for processing (1)
frontend/web/components/IntegrationList.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7498720d-f05b-44b4-b96c-3cf6692192c7
📒 Files selected for processing (1)
api/integrations/amplitude/amplitude.py
| response = requests.post(self.url, data=payload, timeout=10) | ||
| record_integration_health(self.config, response.status_code) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Network/timeout failures never produce a health record.
If requests.post raises (now more likely to time out given the newly added timeout=10, or on ConnectionError), the exception propagates before record_integration_health is reached — so the one case this feature most needs to surface (an unreachable/slow Amplitude endpoint) leaves the integration stuck at "No Health Data" rather than "Unhealthy". This appears to be the same pattern in sibling wrappers (heap, mixpanel), so may be a systemic gap across the cohort rather than amplitude-specific.
♻️ Suggested fix
def _identify_user(self, user_data: AmplitudeUserData) -> None:
payload = {"api_key": self.api_key, "identification": json.dumps([user_data])}
- response = requests.post(self.url, data=payload, timeout=10)
- record_integration_health(self.config, response.status_code)
- logger.debug(
- "Sent event to Amplitude. Response code was: %s" % response.status_code
- )
+ try:
+ response = requests.post(self.url, data=payload, timeout=10)
+ except requests.exceptions.RequestException:
+ logger.exception("Failed to send event to Amplitude.")
+ record_integration_health(self.config, 503)
+ return
+ record_integration_health(self.config, response.status_code)
+ logger.debug(
+ "Sent event to Amplitude. Response code was: %s" % response.status_code
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| response = requests.post(self.url, data=payload, timeout=10) | |
| record_integration_health(self.config, response.status_code) | |
| def _identify_user(self, user_data: AmplitudeUserData) -> None: | |
| payload = {"api_key": self.api_key, "identification": json.dumps([user_data])} | |
| try: | |
| response = requests.post(self.url, data=payload, timeout=10) | |
| except requests.exceptions.RequestException: | |
| logger.exception("Failed to send event to Amplitude.") | |
| record_integration_health(self.config, 503) | |
| return | |
| record_integration_health(self.config, response.status_code) | |
| logger.debug( | |
| "Sent event to Amplitude. Response code was: %s" % response.status_code | |
| ) |
…at are older than 30 days
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
1 participant
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Contributes to #4324
What I understand and how I build this feature:
Users need health tracker for every thirdy-party integration tool that will indicate whether it has showed healthy / unhealthy responses.
We need a specific table (
IntegrationHealthRecord) that holds the information about which third-party tool we integrate with and its health status.I create 2 helper functions:
record_integration_health(): create records in the specific table, if the response is2xx.get_latest_integration_health(): verify if the latest record on a thirdyparty config is presentWhen our server sends payload through a wrapper, that third-party tool will send back a response. That response is what we capture and stores in the special table. I don't include the data payload of the response, to prevent the bulking of database's size.
For every third-party tool, a health indicator on the client will appear with 3 status:
Healthy(2xxresponse),Unhealthy(anything that excludes from 2xx response), andNo Health Data(indicates that integrations hasn't happened, but the user has add that config with base url)For additional information:
latest_healthfield. To mitigate this, I also create unit tests to cover this.How did you test this code?
I create several unit tests that cover various flow:
latest_healthpresent and shaped correctly in the JSON response?)record_integration_healthafter the HTTP call?)CLI:
.venv/bin/pytest --ds=app.settings.test tests/unit/integrations/ -n0Result of testing all of the file:
After changes:
