channels-1094

[cogwizzle]

refactor: Refactoring the name for dataFeedCache to be
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.

A summary of your pull request, including the what change you're making and why.

Testing

Include any additional information about the testing you have completed to
ensure your changes behave as expected. For a speedy review, please check
any of the tasks you completed below during your testing.

• Added unit tests for new functionality
• Tested end-to-end using the local server
• [Segmenters] Tested in the staging environment

[cogwizzle]

Broke this comment apart so that we could get language server support for using each property in the shape of this object.

[cogwizzle]

Broke this comment apart so that we could get language server support for using each property in the shape of this object.

[pmunin]

I might be wrong but reading response.text() here will close the stream and will fail next time response body is tried to be read (during actual logic of processing response), so you might want to check it and if it's the case you may use response.clone() or pass here the actual value to be cached, instead of response object

[cogwizzle]

Since I'm swapping to use the status code I don't think we need to worry about this.

[pmunin]

I'd do it easier and in EngageActionPerformer.ts/perform method:

    const op = this.currentOperation as OperationContext
    // this is similar to defer in golang, will happen during finally of entire perform
    // you can use standard (try-catch-finally instead)
    // during finally we need to cache if it's success or non-retriable error:
    let cache = undefined
    op?.onFinally.push(() => {
      if(cache) return
      const error = op.error as TrackedError
      if(!error) this.setCache(messageId, stringifySuccess())
      else if (error && !isErrorRetryable(error)) this.setCach(messageId, stringifyError(error))
    })
    //now here before starting regular perform logic we check if we have cache 
    // and if we do, we don't need to perform
    try {
      cache = parseCache(this.getCache(messageId)) 
      // will be undefined if not found or throw error if redis issue
    }
    catch(redisError)
    {
        //log problem with redis
        // in that case we proceed as if nothing is cached
    }

    if (isCacheSuccess(cache)) return cache.result
    else if(isCachedError(cache)) throw cache.error

    // existing code of perform:
    beforePerform()
    return doPerform()

[cogwizzle]

Can you help me understand why this belongs in EngageActionPerformer rather than in the MessageSendPerformer? I'm certain you have a reason, but I'm not 100% certain the express purpose of each class.

[pmunin]

EngageActionPerformer is a base class for any engage DA, which contains base logic such as logging, stats, errorhandling, extracting stuff from perform's input args etc.

The only derived class right now is MessageSendPerformer, which is specialized on message sending workflow, i.e. extracting template, personalizing it for profile and then sending it to all recipients.

It's arguable, but I'd suggest for our case it is more practical to dedup on DA.perform level rather than on per recipient level because:

1. most of actions have single recipient (recently we allowed having multiple recipients per email as a workaround for some subscription related sev, but iirc our strategy is to have a single recipient per DA.perform)
2. it is safe to assume that if it message succeeded or failed for one of the recipients - retryability of it will be applicable to the other recipients of that message, because based on what I saw in datadog in 99% of the cases failure happens before hitting TW/SG api call.

Therefore since we're talking about caching of entire DA.perform outcome (and not per each recipient within each perform) for preventing dup calls of DA.perform - that logic belongs to EngageActionPerformer.

Hope it makes sense?

[cogwizzle]

I would like to keep the current implementation for a few reasons.

I tried to implement the suggested pattern you outlined above. From an outsiders perspective the code was much harder for me to interpret and follow due to the unconventional coding style associated with the suggested change. I really struggled due to the unusual types associated with the onFinally code. I don't think casting the type is a reasonable solution either, because I am not familiar enough with the code base to know what the given type is supposed to be.

The existing implementation handles both the multi-recipient and single-recipient case. If we do support the multi-recipient case shouldn't we build a caching implementation that works this way too? I have concerns about potential side effects if we don't use a compound key such as messageId and recipientId we may return an inappropriate cached value which may cause messages to fail to be sent due to cache collisions. In a sense my fear is that we go from creating duplicates to dropping messages in a way that would not be very transparent to trace through.

If you'd like to discuss this further I recommend we pair on this solution to get the most equitable solution between our two proposed implementations. I'm sure together we can come up with a reasonable solution if you decide it is worth going down this road.

[joe-ayoub-segment]

hi @cogwizzle @pmunin please let me know when you are done with this so I can review prior to deploy