Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3456 +/- ##
==========================================
+ Coverage 80.01% 80.07% +0.05%
==========================================
Files 1249 1267 +18
Lines 23127 23368 +241
Branches 4607 4675 +68
==========================================
+ Hits 18505 18711 +206
- Misses 3760 3790 +30
- Partials 862 867 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
551ec43 to
8623abc
Compare
2517f10 to
7daf5df
Compare
| }) | ||
| } catch (error) { | ||
| // Set the default error to Bad Request | ||
| let httpStatusCode = 400 |
There was a problem hiding this comment.
can we make it 500 instead. We drop events for 400
There was a problem hiding this comment.
We keep the default error to 400 so that centrifuge doesn't keep on retrying invalid events. Down in the code we check for additional conditions like the actual error response from AWS APIs / client side errors. We overwrite them with 500 for those errors.
There was a problem hiding this comment.
The behaviour here will be that if we don't receive httpStatusCode in error?.$metadata?.httpStatusCode maybe because the service behaved differently or hit some edge case, we will end up dropping events.
It doesn't sounds good to me because the default behaviour should be 500 (with retry) and not 400.
There was a problem hiding this comment.
Pull request overview
This PR adds multistatus support to the Trade Desk CRM destination to enable better observability of dropped events. The implementation validates email formats in payloads and uses multistatus responses to track which events succeed or fail, replacing the previous approach of throwing errors.
Changes:
- Modified the
processPayloadfunction to returnMultiStatusResponseinstead of throwing errors - Added email validation with multistatus error tracking for invalid email formats
- Updated error handling for both legacy and AWS flows to use multistatus responses
- Enhanced test coverage with multistatus validation for error scenarios
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/destination-actions/src/destinations/the-trade-desk-crm/functions.ts | Refactored to use MultiStatusResponse for error tracking; added email validation error handling; converted thrown errors to multistatus error responses |
| packages/destination-actions/src/destinations/the-trade-desk-crm/syncAudience/index.ts | Added await keyword to processPayload calls for consistency (no functional change) |
| packages/destination-actions/src/destinations/the-trade-desk-crm/syncAudience/tests/index.test.ts | Added MultiStatusErrorNode interface and new test for invalid email multistatus handling; updated batch size validation test to check multistatus instead of thrown errors |
packages/destination-actions/src/destinations/the-trade-desk-crm/functions.ts
Show resolved
Hide resolved
packages/destination-actions/src/destinations/the-trade-desk-crm/functions.ts
Show resolved
Hide resolved
packages/destination-actions/src/destinations/the-trade-desk-crm/functions.ts
Show resolved
Hide resolved
packages/destination-actions/src/destinations/the-trade-desk-crm/functions.ts
Show resolved
Hide resolved
...destination-actions/src/destinations/the-trade-desk-crm/syncAudience/__tests__/index.test.ts
Show resolved
Hide resolved
be38eff to
f73f8df
Compare
This change adds multistatus support to the-tradedesk-crm action destination. This change is done to support observability changes. The framework validates the the emails in the payloads and drops the payloads with invalid emaild ids. Multistatus is used to track all the dropped events.
Testing
Testing in Stage
Testing in Local
Integrations monoservice was run locally with a dummy cloudevent payload.
Test Events fromCloudevent Spec
{ "channel": "server", "messageId": "segment-test-message-000001", "timestamp": "2023-07-26T15:23:39.803Z", "type": "track", "email": "test-user-1@example.com", "userId": "userid1", "receivedAt": "2015-12-12T19:11:01.266Z", "properties": {}, "event": "Audience Entered" }, { "channel": "server", "messageId": "segment-test-message-000002", "timestamp": "2023-07-26T15:23:39.803Z", "type": "track", "email": "bad-example", "userId": "userid2", "receivedAt": "2015-12-12T19:11:01.266Z", "properties": {}, "event": "Audience Entered" }, { "channel": "server", "messageId": "segment-test-message-000003", "timestamp": "2023-07-26T15:23:39.803Z", "type": "track", "email": "test-user-3@example.com", "userId": "userid3", "receivedAt": "2015-12-12T19:11:01.266Z", "properties": {}, "event": "Audience Entered" }Response from monoservice
{ "responses": [ { "status": 200, "events": [ { "indices": [ 0, 2 ], "data": "Successfully Uploaded to S3" } ] }, { "status": 400, "events": [ { "indices": [ 1 ], "errortype": "PAYLOAD_VALIDATION_FAILED", "errormessage": "Invalid email format", "errorreporter": "INTEGRATIONS" } ] } ] }Security Review
Please ensure sensitive data is properly protected in your integration.
type: 'password'