[MAIN] [STRATCONN] 4261 Mixpanel Multistatus support

[AnkitSegment]

Multistatus support is added for Mixpanel destinations
JIRA TICKET: https://segment.atlassian.net/browse/STRATCONN-4261

Testing Docs

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
• [If destination is already live] Tested for backward compatibility of destination. Note: New required fields are a breaking change.
• [Segmenters] Tested in the staging environment
• [Segmenters] [If applicable for this change] Tested for regression with Hadron.

[codecov]

Codecov Report

Attention: Patch coverage is 95.83333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 78.58%. Comparing base (dda2c53) to head (db5f9e6).
Report is 78 commits behind head on main.

Files with missing lines	Patch %	Lines
...-actions/src/destinations/mixpanel/common/utils.ts	85.71%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2536      +/-   ##
==========================================
+ Coverage   78.00%   78.58%   +0.58%     
==========================================
  Files         991     1030      +39     
  Lines       17388    18229     +841     
  Branches     3281     3441     +160     
==========================================
+ Hits        13563    14325     +762     
- Misses       2737     2759      +22     
- Partials     1088     1145      +57     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[joe-ayoub-segment]

hi @AnkitSegment and @varadarajan-tw .

Ankit and I just reviewed PR with @spjtls9 from Mixpanel.
2 main suggestions were:

1. Can we keep the perform() and performBatch() functions using the exact same code path?
   • i.e they call the same functions, with an array of payloads being passed in.
2. Can we put the new code path and old code paths behind a feature flag, and deploy this code dark, then roll out in a controlled manner?
   • This will allow for testing in Production, and for a quick and safe rollback should issues arise.

[AnkitSegment]

hi @AnkitSegment and @varadarajan-tw .

Ankit and I just reviewed PR with @spjtls9 from Mixpanel. 2 main suggestions were:

1. Can we keep the perform() and performBatch() functions using the exact same code path?

   • i.e they call the same functions, with an array of payloads being passed in.

2. Can we put the new code path and old code paths behind a feature flag, and deploy this code dark, then roll out in a controlled manner?

   • This will allow for testing in Production, and for a quick and safe rollback should issues arise.

Hey @joe-ayoub-segment ,

1. I've updated the code to ensure that perform() and performBatch() now follow the same code path. Both functions pass an array of payloads to the same underlying logic, as requested.

2. We already have the cloudevents-spec-v2-all feature flag, which controls multistatus responses based on user tiers. This allows for a controlled rollout and testing in production.
   If you think we need a separate or more granular flag separately for this destination, I’m happy to discuss and make the necessary adjustments.

[joe-ayoub-segment]

hi @AnkitSegment ,
Just had a look through the code.
I'm not familiar with Hubspot's API or this Destination so I'm not able to properly review. But I do have a couple of questions I'm sure you can easily answer ;).

Question 1: The the handleMixPanelApiResponse function, why is this condition present?

if (apiResponse.data.code === 400 || apiResponse.data.code === 200) {
// do stuff
}
if (apiResponse.data.code !== 200 && apiResponse.data.code !== 400) {
// do other stuff
}

Is it because If a 400 response is returned it's still possible that some events were successfully delivered? And conversely, if a 200 response is returned then there can also be some failures?

Question 2: for the trackPurchase function, why are there 2 arrays, events and sentEvents?

Why is this happening?

sentEvents.push(purchaseEvents as object as JSONLikeObject)
events.push(...purchaseEvents)

I assume sentEvents is just a duplicate array which keeps track of the events which were actually sent (so you have a reference for the index of each event when you process the response)?

[joe-ayoub-segment]

Also did you make a decision regarding how to roll this out? Will it be behind a feature flag?

[AnkitSegment]

Hello @joe-ayoub-segment ,
Answer 1. We have different conditions for statuses 400 and 200 because the response can either indicate full success or partial success with failed records. Any other response would mean the entire batch has failed.

Answer 2. Yes, sendEvents is to track the events that were actually sent.

[varadarajan-tw]

LGTM. Thanks for adding the feature flag.
Tagging @joe-ayoub-segment to see if he has any other comments.

[joe-ayoub-segment]

hi @AnkitSegment thanks for making the update to add the FF.
It still looks to me like some new code paths are being introduced even for users who have the flag set to off. Is there a deliberate reason for this?

[joe-ayoub-segment]

hi @AnkitSegment please let me know when you'd like me to rereview.