[STRATCON-6723] Retryable / Non Retryable Error Mappings for Marketo #3733
[STRATCON-6723] Retryable / Non Retryable Error Mappings for Marketo #3733joe-ayoub-segment merged 9 commits intomainfrom
Conversation
fd8e704 to
1274678
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3733 +/- ##
==========================================
- Coverage 81.08% 80.90% -0.18%
==========================================
Files 1655 1347 -308
Lines 32079 25048 -7031
Branches 7070 5194 -1876
==========================================
- Hits 26011 20266 -5745
+ Misses 5096 3837 -1259
+ Partials 972 945 -27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
packages/destination-actions/src/destinations/marketo-static-lists/functions.ts:329
607(“Daily quota exhausted”) is included inMARKETO_RETRYABLE_CODES, which will cause retries even though this condition typically won't succeed until the quota resets. If this code is meant to be non-retryable (as described in the PR), remove it from the retryable set and handle it in the non-retryable mapping instead.
'606', // Rate limit exceeded (>100 calls per 20 seconds)
'607', // Daily quota exhausted
'608', // API temporarily unavailable
packages/destination-actions/src/destinations/marketo-static-lists/functions.ts:337
MARKETO_RETRYABLE_CODESwas expanded (e.g., adding500and1016), but the existing unit tests only exercise a fixed list of codes. Please extend the retryable-code test matrix to include the newly-added codes so future changes to this set are covered and don't regress silently.
const MARKETO_RETRYABLE_CODES = new Set([
'500', // Internal server error
'502', // Bad gateway / timeout
'604', // Request timeout
'606', // Rate limit exceeded (>100 calls per 20 seconds)
'607', // Daily quota exhausted
'608', // API temporarily unavailable
'611', // Unhandled system exception
'614', // Unreachable subscription
'615', // Concurrent access limit exceeded
'713', // Transient error – system resource temporarily unavailable
'719', // Lock wait timeout
'1019', // Batch import in progress on list
'1016', // Too many imports in progress
'1029' // Queue/export limit exceeded
| } | ||
|
|
||
| throw new IntegrationError(message, ErrorCodes.RETRYABLE_ERROR, 500) | ||
| throw new IntegrationError(message, ErrorCodes.BAD_REQUEST, 400) |
There was a problem hiding this comment.
Do we want to invert this condition like we last discussed ? - By default all error error codes are retryable and we decide which ones are not ? cc: @mdkhan-tw
There was a problem hiding this comment.
Yes, the right behaviour is we retry for unhandled error codes.
There was a problem hiding this comment.
Yes, but the list of non-retryable error codes is quite long. The 61x, 7xx, and 10xx ranges alone add up to more than 20 codes. Apart from API level custom code from marketo, 61Xs / 7XXs are response level errors and 1XXXs are record level errors.
https://experienceleague.adobe.com/en/docs/marketo-developer/marketo/rest/error-codes
There was a problem hiding this comment.
Sure, if we don't want to handle so many error codes, we can handle important error codes (like auth , bad payload) which we are doing already. For every other unhandled error, we should retry.
Can you please let us know the reason why we are making this change?
There was a problem hiding this comment.
Hey, ive audited the error codes which are non retryable and sticking with the approach as discussed i.e identifying the known Non retryable error codes and retrying for all others.
| @@ -351,11 +394,11 @@ function parseErrorResponse(response: MarketoResponse) { | |||
| throw new IntegrationError(message, 'INVALID_AUTHENTICATION', 401) | |||
There was a problem hiding this comment.
In parseErrorResponse, auth failures use the literal 'INVALID_AUTHENTICATION' string while the batch path uses ErrorCodes.INVALID_AUTHENTICATION. Using the shared constant here as well keeps error typing consistent and avoids accidental typos/drift.
| throw new IntegrationError(message, 'INVALID_AUTHENTICATION', 401) | |
| throw new IntegrationError(message, ErrorCodes.INVALID_AUTHENTICATION, 401) |
… error codes for single event
| @@ -351,11 +394,11 @@ function parseErrorResponse(response: MarketoResponse) { | |||
| throw new IntegrationError(message, 'INVALID_AUTHENTICATION', 401) | |||
There was a problem hiding this comment.
In parseErrorResponse, auth failures (601/602) throw new IntegrationError(message, 'INVALID_AUTHENTICATION', 401) while the batch path uses ErrorCodes.INVALID_AUTHENTICATION. Even if the string currently matches, this inconsistency makes it easier to regress and complicates global handling that keys off ErrorCodes. Use ErrorCodes.INVALID_AUTHENTICATION here as well for consistency.
| throw new IntegrationError(message, 'INVALID_AUTHENTICATION', 401) | |
| throw new IntegrationError(message, ErrorCodes.INVALID_AUTHENTICATION, 401) |
| it.each([ | ||
| '603', | ||
| '605', | ||
| '609', | ||
| '610', | ||
| '612', | ||
| '613', |
There was a problem hiding this comment.
The non-retryable Marketo code list is duplicated across multiple it.each([...]) blocks in this test file (and partially duplicated again for removeFromList). This makes it easy for the tests to drift from MARKETO_NON_RETRYABLE_CODES in functions.ts. Consider extracting the code arrays into a shared constant (or importing from the implementation file if feasible) and reusing it across the test cases.
mdkhan-tw
left a comment
There was a problem hiding this comment.
Let's monitor it during prod deployment.
|
deployed |
JIRA Ticket: https://twilio-engineering.atlassian.net/browse/STRATCONN-6723
This pull request makes several important updates to the Marketo Static Lists integration, focusing on improving error handling and adjusting batching behavior. The most significant changes include enforcing a minimum batch size, refining error status codes and error types for payload validation failures, and updating the set of retryable error codes.
Marketo Erorr Codes Guide: https://experienceleague.adobe.com/en/docs/marketo-developer/marketo/rest/error-codes
Response-Level Error Codes
%s; exceeded with in%ssecscontent type: application/json. See this StackOverflow question for more details.success = trueand no errors and set a warnings informational string.ExternalSalesPersonIDvalue.Record-Level Error Codes
filterTypespecified with unsupported standard fields (ex: firstName, lastName)cookieparameter. This also occurs when calling Get Leads by Filter Type withfilterType=cookiesand an invalid value for thefilterValuesparameter.cloneToProgramNamein the Schedule Program for the daySFDC FieldlengthSFDC Fieldexceeding the limit of allowed characters. To correct, reduce the length ofSFDC Field, or set mergeInCRM to false.Retryable — bucketed status mapping to 500 RETRYABLE_ERROR
Testing
Tested in staging syncing 65k audience
Dekivery Overview: https://app.segment.build/arjun-test/destinations/actions-marketo-static-lists/sources/personas_arjun-test/instances/699e9cb4bba622800aa0b8b4/event-delivery/RETRYABLE_ERROR?period=past-day
Audience Syncs
Failures with 1006 Status code as Retryable Error
Datadog Errors graph
Security Review
Please ensure sensitive data is properly protected in your integration.
type: 'password'New Destination Checklist
verioning-info.tsfile. example