Channels-1094 refactored #2325
Channels-1094 refactored #2325
Conversation
engageDestinationCache. This is because this cache is no longer just intended to be used for DataFeed cache, but is now intended to be used as a more generic cache for the Engage Destination.
… into channels-1094
… into channels-1094
… into channels-1094
… into channels-1094
action-destination project is easier.
… into channels-1094
the JavaScript event loop for as long.
… into channels-1094
SendEmailPerformer.
|
|
||
| const DEFAULT_RETRY_ATTEMPTS = 2 | ||
|
|
||
| export async function retry<T>( |
There was a problem hiding this comment.
This looks like you copied it from actions-core. Please resolve the import at the top and avoid duplicating code entirely
There was a problem hiding this comment.
This is exactly what the comment above says:
/**
* import { retry } from '@segment/actions-core/retry'
* ^ This import is not working, gives a runtime error: retry_1.retry is not a function during jest tests,
* so the following code is a copy of the original code from: @segment/actions-core/retry.
* Once jest config is fixed to use the correct import, the following code should be removed.
*/
This is for owners of the repo to resolve in a separate PR. has nothing to do with current story.
.../destination-actions/src/destinations/engage-messaging-sendgrid/__tests__/send-email.test.ts
Outdated
Show resolved
Hide resolved
.../destination-actions/src/destinations/engage-messaging-sendgrid/__tests__/send-email.test.ts
Outdated
Show resolved
Hide resolved
...stination-actions/src/destinations/engage-messaging-sendgrid/sendEmail/SendEmailPerformer.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Read thru all the code, thank you for addressing comments so far.
I do think for the getOrRetry.ts file, the broken import: // import { retry } from '@segment/actions-core/retry' // TODO: fix this import. Currently it gives a runtime error: retry_1.retry is not a function (see below) needs to be fixed before merging.
If you can't find a way to get that working then please write up a ticket on the stratconn board that describes how to reproduce the issue you're seeing so that we can be aware of it. We shouldn't leave TODOs in the codebase without a solid way to followup on them. Please don't leave items for our team to fix without notifying us of the issue
|
@nick-Ag fixed it (it was easier to fix than I expected), plz review |
| try { | ||
| const msgs = [msg, ...(metadata ? [JSON.stringify(metadata)] : [])] | ||
| const [firstMsg, ...rest] = msgs | ||
| this.loggerClient?.info( |
There was a problem hiding this comment.
Do we need to leave these logs around permanently? If they're to facilitate testing, can we commit to removing them once the changes are validated in production?
There was a problem hiding this comment.
yes, we need it. this is old logic - logInfo is feature-flagged and by default OFF in Production. We occasionally turn it on for certain workspaces for troubleshooting scenarios. The change in this PR is to wrap it with try-catch, to reduce risks of logging impacting duplicate rate (we see during tests that with logInfo enabled dup rate is higher for some reason)
|
hi @brennan there's some core changes in here. Do you want me to deploy this on Tuesday, or would you prefer that it's done by someone in the US during US working hours so that it can be monitored more closely? |
|
@joe-ayoub-segment Please do not merge/deploy this PR as part of the regularly scheduled deploy on October 8. The on-call will deploy this separately in the late morning/early afternoon. Thank you. |
Must be deployed together with Integrations PR. Please see deployment and testing information there.
Refactoring changes of #2024
Implementation of Dedup RFC
Testing
Test results
With Logging OFF - introducing Caching reduces dups from ~3k down to 184 (~94%)
With Logging ON - Caching reduces dups from 4.5k down to 2.3k (~49%)