Pass eventOccurredTS value to the custom_args of the sendgrid/twilio body #1955
Conversation
|
hi @miguelpdiaz8 please let me know when someone on your team has reviewed, and when you are happy for me to review+deploy. |
Hi @joe-ayoub-segment I just shared it with the team, and you can review it too, thanks |
packages/destination-actions/src/destinations/engage/twilio/__tests__/send-mobile-push.test.ts
Outdated
Show resolved
Hide resolved
packages/destination-actions/src/destinations/engage/twilio/utils/TwilioMessageSender.ts
Outdated
Show resolved
Hide resolved
| @@ -121,6 +122,7 @@ export abstract class TwilioMessageSender<TPayload extends TwilioPayloadBase> ex | |||
| const customArgs: Record<string, string | undefined> = { | |||
| ...this.payload.customArgs, | |||
| space_id: this.settings.spaceId, | |||
| event_occurred_ts: this.payload?.eventOccurredTS, | |||
There was a problem hiding this comment.
You missed assigning adding it to Tags - which is used by Event Streams:
Even though we do pass that "webhook" (i.e. callback Url) - I don't believe it is used anymore. So most important is add it to tags. However you need to make sure you're not exceeding tag limits - I think it was up to 10 tags max.
There was a problem hiding this comment.
Yes, I had the tags change but changed it back, I was waiting to ask you about the tags limit knowing it exists and it currently has 10 tags already, do you know which one I could remove safely, or if that limit can be changed? Thanks Philipp
|
hi @miguelpdiaz8 @pmunin @cogwizzle - just a reminder to let me know when I can review / deploy |
|
Looks good. I'll defer to @pmunin as he has requested additional changes. |
|
Can you confirm if adding a new field within custom_args impacts the processing of current event payload with [engage-events-ingest service? In theory, it shouldn't. But in the past, we have seen payloads get dropped (4xx response) due to bad format. |
So you are saying there is no a hard limit of 10 custom args? @rahul8590 |
|
hi @miguelpdiaz8 @pmunin @rahul8590 @cogwizzle - friendly reminder to let me know when this is ready for deploy. |
|
hi @miguelpdiaz8 - please let me know when this PR is ready for review / and then to be deployed. |
In the DestinationAction payload we receive eventOccurredTS, which we use to stats event delivery latency which is for some reason called eventDeliveryTS. Regardless of confusing name - this metric helps to understand how much time it took from the event creation till the end of event processing by Destination Action.
It would be helpful to pass that eventOccurredTS value to the custom_args of the sendgrid/twilio body, so it will be returned to a webhook call after email/sms is delivered and we can add that [twilio|sendgrid]_eventDeliveryDuration metric to engage-events-ingest service (supplying all the tags that twilio_total and sendgrid_total have) to track how much time it takes for email/sms to be delivered from the moment of placing it to centrifuge to the moment of delivering it to the user.
Testing
Updated unit tests