Skip to content

feat: add MNTN Audiences destination#3796

Closed
valerie-mntn wants to merge 3 commits into
segmentio:mainfrom
valerie-mntn:add-mntn-audiences-destination
Closed

feat: add MNTN Audiences destination#3796
valerie-mntn wants to merge 3 commits into
segmentio:mainfrom
valerie-mntn:add-mntn-audiences-destination

Conversation

@valerie-mntn

Copy link
Copy Markdown

Adding a new Action Destination for MNTN Audiences. An integration to facilitate the sending of audiences data from Segment to MNTN. This action destination calls the MNTN Audience API with a customer-owned API key from MNTN to authenticate requests for audiences adds and audience removals.

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.

  • [X ] Added unit tests for new functionality
  • [X ] Tested end-to-end using the local server
  • [N/A] [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.

Security Review

Please ensure sensitive data is properly protected in your integration.

  • [X ] Reviewed all field definitions for sensitive data (API keys, tokens, passwords, client secrets) and confirmed they use type: 'password'

New Destination Checklist

  • [N/A ] Extracted all action API versions to verioning-info.ts file. example

@joe-ayoub-segment

Copy link
Copy Markdown
Contributor

Hi @valerie-mntn - thanks for raising the PR for a new audience destination.
Before I do the review I need to point out that you should use a single Action which handles both adding and removing a user from an Audience. There are some examples of separate Actions in the repo, but these are being migrated over to single Actions.

@joe-ayoub-segment

Copy link
Copy Markdown
Contributor

Hi @valerie-mntn - thanks for raising the PR for a new audience destination.

I had a quick scan and noticed some big items I'd like you to address please.

  1. Please use a single Action which handles both adding and removing a user from an Audience. There are some examples of separate Actions in the repo, but these are being migrated over to single Actions.

  2. You'll need to change how you figure out audience membership. The correct way to do this is to look at the context.personas.computation_key value, then to take that and look in the properties[<computation_key>] location in the payload. This sounds more confusing than it is, so I'll instead point to a dimple destination which already does this.

  3. Consider implementing the performBatch() function to handle batches of events. Audiences tend to send bursty traffic, which might overwhelm your API.

  4. If you do implement performBatch(), if your API returns 207 responses (with multiple statuses), you could also return a multistatusResponse in the performBatch() function. This helps a lot with observability in the Segment UI.

Have a look at this destination: https://github.com/segmentio/action-destinations/tree/main/packages/destination-actions/src/destinations/iterable-audiences

It does all 1-3 from above, and partially does 4 (Iterable doesn't actually respond with a 207, but we're still returning a multistatusResponse containing validation failures and successful requests).

If you can familiarize yourself with the Iterable audiences destination code - and have a go at updating your code, then I will do a full review.

If you need help ping me and we can jump on a call.

best regards,
Joe

@valerie-mntn

Copy link
Copy Markdown
Author

Hi @joe-ayoub-segment thanks for the feedback! Made the requested changes. Let me know if anything else is needed

@joe-ayoub-segment

Copy link
Copy Markdown
Contributor

HI @valerie-mntn ,

I copied your code to another branch and refactored the destination. I'm going to close this current PR.

Here's the new PR: #3809

The changes include:

  1. More typing
  2. Separating functions, types into separate files
  3. Merging functionality across perform and performBatch
  4. updated unit tests

I tried to keep the core logic as per your original code, so I'm hoping it will make sense to you.

As a next step, would you be able to run the code and check that it looks good please? You can raise changes into this branch if you like, and I'll merge them in.

I'm available to jump on a call if that will be quicker.

Best regards,
Joe

@joe-ayoub-segment

Copy link
Copy Markdown
Contributor

Hi @valerie-mntn - commenting here again in case you missed my last comment.
There are some comments for you to respond to on the refactored version of your PR here: #3809

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants