feat(integration): Add health-tracking integration feature#7956
feat(integration): Add health-tracking integration feature#7956afsuyadi wants to merge 64 commits into
Conversation
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. |
|
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/integrations/common/services.py (1)
19-29: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a tie-breaker to
order_byfor deterministic "latest" selection.Ordering only by
-created_atcan be ambiguous if two records share the same timestamp (e.g. rapid successive writes with coarse timestamp resolution). Add the primary key as a secondary sort key for a deterministic result.🩹 Proposed fix
.order_by("-created_at") + .order_by("-created_at", "-pk") .first()
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bb7f5ec4-6b0f-4a0c-baf6-e19aaf7ee17e
📒 Files selected for processing (2)
api/integrations/common/serializers.pyapi/integrations/common/services.py
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: 1cf11e43-8ca2-475f-a16f-e4d271e802c0
📒 Files selected for processing (2)
api/integrations/common/services.pyapi/integrations/new_relic/new_relic.py
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 | |
| ) |
|
Hi @emyller , I have created this PR for Health Tracking feature. Is there anything more need to be added or modified? Thanks! |
Hi @afsuyadi — thanks for the work done so far, the screenshots look great! Sorry we're lacking the opportunity to review this timely, as we're doing internal pushes on internal goals. This is quite a chunky contribution in size, so bear with us a little there. 😉 At a glance, I see there are some typing issues related to your changes according to CI. Would you please take a look whenever you have a chance? |
…ration serializer
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
afsuyadi
force-pushed
the
feat/track-integration-health
branch
from
85480bd to
a98cb12
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7956 +/- ##
==========================================
- Coverage 98.64% 98.52% -0.12%
==========================================
Files 1501 1508 +7
Lines 59397 59880 +483
==========================================
+ Hits 58591 58998 +407
- Misses 806 882 +76 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
No prob, please take your time @emyller . I have addressed all of them (API tests, frontend linting, and Codecov's reviews). Please let me know if there are more that needed to be changed. |
2 participants
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:
