Skip to content

Introduce 'Amazon Conversions Api' Destination #2941

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

bansrav
Copy link

@bansrav bansrav commented May 23, 2025

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

We are introducing a new destination action to Integrate with Amazon Conversion API. The Amazon Conversions API integration allows Segment customers to stream conversion event data directly to Amazon's advertising platforms. This server-side integration ensures reliable data delivery, enables accurate attribution for advertising campaigns. Advertisers will benefit from this integration to accurately measure how their campaigns are performing.
The new destination will look like
Screenshot 2025-05-13 at 4 40 14 PM

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.

Local Testing on Segment UI
Screenshot 2025-05-23 at 3 04 22 PM
Local Testing on via postman API
Screenshot 2025-05-23 at 3 06 18 PM

  • 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.

Comment on lines 144 to 171
export interface SubErrorV1 {
errorCode?: number
errorType?: string
errorMessage?: string
}

export interface BatchError {
httpStatusCode?: string
itemRequestId?: string
index: number
itemId?: string
subErrors?: SubErrorV1[]
}

export interface ImportEventBatchSuccess {
index: number
}

export interface EventDataSuccessResponseV1 extends ImportEventBatchSuccess {
message?: string
}

export interface EventDataErrorResponseV1 extends BatchError { }

export interface ImportConversionEventsResponse {
success?: EventDataSuccessResponseV1[]
error?: EventDataErrorResponseV1[]
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tidying up these types

Copy link
Contributor

@joe-ayoub-segment joe-ayoub-segment May 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export interface SubErrorV1 {
errorCode?: number
errorType?: string
errorMessage?: string
}
export interface BatchError {
httpStatusCode?: string
itemRequestId?: string
index: number
itemId?: string
subErrors?: SubErrorV1[]
}
export interface ImportEventBatchSuccess {
index: number
}
export interface EventDataSuccessResponseV1 extends ImportEventBatchSuccess {
message?: string
}
export interface EventDataErrorResponseV1 extends BatchError { }
export interface ImportConversionEventsResponse {
success?: EventDataSuccessResponseV1[]
error?: EventDataErrorResponseV1[]
}
export interface BatchResponse extends Response {
data?: {
success?: SuccessResponse[]
error?: ErrorResponse[]
}
}
export interface SuccessResponse {
index: number
message?: string
}
export interface ErrorResponse {
httpStatusCode?: string
itemRequestId?: string
index: number
itemId?: string
subErrors?: {
errorCode?: number
errorType?: string
errorMessage?: string
}[]
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I flattened the response structure as you suggested but I don't want the naming to be changed to BatchResponse due to following reasons:

  1. It's not really a batch response only, our actual API response is same for both perform and performBatch so naming it as BatchResponse won't go well when its used within the perform function context for ex: inside perform I use it as return await sendEventsRequest<ImportConversionEventsResponse>
  2. I prefer to use the naming structure matching with our API response similar to what we have used for request also (EventData) because this semantically is tied to Amazon API response and gives more context to developers working on it in future.

My response to other suggestions here:

  1. We don't need to do extend Response here and introduce data entity for BatchResponse because I need to use ModifiedResponse<> which anyways extends Response (refer to my other comment for details)
  2. I prefer to keep subErrors definition separate and not flatten within Errorresponse

import { IntegrationError, RequestClient, APIError, MultiStatusResponse, JSONLikeObject } from '@segment/actions-core'
import { processHashing } from '../../../lib/hashing-utils'
import type { Settings } from '../generated-types'
import type { EventData, ConsentData, RegionValue, AmazonConsentFormat, ImportConversionEventsResponse, EventDataSuccessResponseV1, EventDataErrorResponseV1, MatchKeyV1, ConversionTypeV2, CurrencyCodeV1, CustomAttributeV1 } from '../types'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import type { EventData, ConsentData, RegionValue, AmazonConsentFormat, ImportConversionEventsResponse, EventDataSuccessResponseV1, EventDataErrorResponseV1, MatchKeyV1, ConversionTypeV2, CurrencyCodeV1, CustomAttributeV1 } from '../types'
import type { EventData, ConsentData, RegionValue, AmazonConsentFormat, BatchResponse, SuccessResponse, ErrorResponse, MatchKeyV1, ConversionTypeV2, CurrencyCodeV1, CustomAttributeV1 } from '../types'

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed, refer to my other comment for why

Comment on lines +199 to +200
const successMap: Record<number, EventDataSuccessResponseV1> = {};
const errorMap: Record<number, EventDataErrorResponseV1> = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const successMap: Record<number, EventDataSuccessResponseV1> = {};
const errorMap: Record<number, EventDataErrorResponseV1> = {};
const successMap: Record<number, SuccessResponse> = {}
const errorMap: Record<number, ErrorResponse> = {}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed, refer to my other comment for why

Comment on lines 300 to 312
dataProcessingOptions: {
label: 'Data Processing Options',
description: 'A list of flags for signaling how an event shall be processed. Events marked for limited data use will not be processed.',
type: 'string',
multiple: true,
choices: [
{ label: 'Limited Data Use', value: 'LIMITED_DATA_USE' }
],
required: false,
default: {
'@path': '$.properties.dataProcessingOptions'
}
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we met a couple of days ago we forgot to come up with a solution for the dataProcessingOptions field.
The way it is written now means that the entire event will be dropped unless dataProcessingOptions: ['LIMITED_DATA_USE'] is passed in the payload.

I propose we do the following:

  dataProcessingOptions: {
    label: 'Data Processing Options',
    description: 'Flags to signal how an event will be processed.',
    type: 'object',
    required: false,
    additionalProperties: false,
    properties: {
      limitedDataUse: {
        label: 'Limited Data Use',
        description: "Events marked for 'Limited Data Use' will not be processed.",
        type: 'boolean',
        required: false
      },
      otherDataProcessingOptions: {
        label: 'Other Data Processing Options',
        description: 'A string array or comma delimited list of other data processing options to apply to the event.',
        type: 'string',
        multiple: true,
        required: false
      }
    },
    default: {
      limitedDataUse: false
    }
  },

Copy link
Author

@bansrav bansrav May 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In our spec, it is modeled as enum and there might be new enums in the future. I don't think modeling it as otherDataProcessingOptions is right solution here.

dataProcessingOptions:
          type: array
          minItems: 0
          maxItems: 1
          items:
            type: string
            enum:
              - LIMITED_DATA_USE
          description: A list of flags for signaling how an event shall be processed. Events marked for limited data use will not be processed.

The way it is written now means that the entire event will be dropped unless dataProcessingOptions: ['LIMITED_DATA_USE'] is passed in the payload

Yes, this is expected. They need to pass what is defined in enum but if they don't populate this field at all, it will still work as the field is not required. We have similar enum modeling in other fields as well, so curious why this field needs a special treatment?

Comment on lines +499 to +536
it('should include optional fields when provided', () => {
const payload = {
name: 'purchase_event',
eventType: ConversionTypeV2.OFF_AMAZON_PURCHASES,
eventActionSource: 'WEBSITE',
countryCode: 'US',
timestamp: '2023-01-01T12:00:00Z',
value: 99.99,
currencyCode: 'USD',
unitsSold: 2,
clientDedupeId: 'dedup-123',
dataProcessingOptions: ['LIMITED_DATA_USE'],
matchKeys: {
email: '[email protected]'
},
consent: {
ipAddress: '1.2.3.4',
amznAdStorage: 'GRANTED',
amznUserData: 'GRANTED'
},
customAttributes: [
{ name: 'color', dataType: 'STRING', value: 'blue' },
{ name: 'size', dataType: 'STRING', value: 'medium' }
],
enable_batching: true
}

const result = prepareEventData(payload, settings)

// Check all optional fields are included
expect(result.value).toBe(99.99)
expect(result.currencyCode).toBe('USD')
expect(result.unitsSold).toBe(2)
expect(result.clientDedupeId).toBe('dedup-123')
expect(result.dataProcessingOptions).toEqual(['LIMITED_DATA_USE'])
expect(result.consent).toBeDefined()
expect(result.customAttributes).toEqual(payload.customAttributes)
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it('should include optional fields when provided', () => {
const payload = {
name: 'purchase_event',
eventType: ConversionTypeV2.OFF_AMAZON_PURCHASES,
eventActionSource: 'WEBSITE',
countryCode: 'US',
timestamp: '2023-01-01T12:00:00Z',
value: 99.99,
currencyCode: 'USD',
unitsSold: 2,
clientDedupeId: 'dedup-123',
dataProcessingOptions: ['LIMITED_DATA_USE'],
matchKeys: {
email: '[email protected]'
},
consent: {
ipAddress: '1.2.3.4',
amznAdStorage: 'GRANTED',
amznUserData: 'GRANTED'
},
customAttributes: [
{ name: 'color', dataType: 'STRING', value: 'blue' },
{ name: 'size', dataType: 'STRING', value: 'medium' }
],
enable_batching: true
}
const result = prepareEventData(payload, settings)
// Check all optional fields are included
expect(result.value).toBe(99.99)
expect(result.currencyCode).toBe('USD')
expect(result.unitsSold).toBe(2)
expect(result.clientDedupeId).toBe('dedup-123')
expect(result.dataProcessingOptions).toEqual(['LIMITED_DATA_USE'])
expect(result.consent).toBeDefined()
expect(result.customAttributes).toEqual(payload.customAttributes)
})
it('should include optional fields when provided', () => {
const payload = {
name: 'purchase_event',
eventType: ConversionTypeV2.OFF_AMAZON_PURCHASES,
eventActionSource: 'WEBSITE',
countryCode: 'US',
timestamp: '2023-01-01T12:00:00Z',
value: 99.99,
currencyCode: 'USD',
unitsSold: 2,
clientDedupeId: 'dedup-123',
dataProcessingOptions: {
limitedDataUse: true,
otherDataProcessingOptions: ['ANOTHER_PROCESSING_OPTION']
},
matchKeys: {
email: '[email protected]'
},
consent: {
ipAddress: '1.2.3.4',
amznAdStorage: 'GRANTED',
amznUserData: 'GRANTED'
},
customAttributes: [
{ name: 'color', dataType: 'STRING', value: 'blue' },
{ name: 'size', dataType: 'STRING', value: 'medium' }
],
enable_batching: true
}
const result = prepareEventData(payload, settings)
// Check all optional fields are included
expect(result.value).toBe(99.99)
expect(result.currencyCode).toBe('USD')
expect(result.unitsSold).toBe(2)
expect(result.clientDedupeId).toBe('dedup-123')
expect(result.dataProcessingOptions).toEqual(['ANOTHER_PROCESSING_OPTION','LIMITED_DATA_USE'])
expect(result.consent).toBeDefined()
expect(result.customAttributes).toEqual(payload.customAttributes)
})

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating the test to work with new shape of dataProcessingOptions field

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed, refer to my other comment for why

Comment on lines +42 to +43
expect(fields.dataProcessingOptions?.type).toBe('string')
expect(fields.dataProcessingOptions?.multiple).toBe(true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
expect(fields.dataProcessingOptions?.type).toBe('string')
expect(fields.dataProcessingOptions?.multiple).toBe(true)
expect(fields.dataProcessingOptions?.type).toBe('object')

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed, refer to my other comment for why

Comment on lines +68 to +71
// Data processing options choices
expect(fields.dataProcessingOptions?.choices).toHaveLength(1)
const choice = fields.dataProcessingOptions?.choices?.[0]
expect(typeof choice === 'object' ? choice.value : choice).toBe('LIMITED_DATA_USE')
Copy link
Contributor

@joe-ayoub-segment joe-ayoub-segment May 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Data processing options choices
expect(fields.dataProcessingOptions?.choices).toHaveLength(1)
const choice = fields.dataProcessingOptions?.choices?.[0]
expect(typeof choice === 'object' ? choice.value : choice).toBe('LIMITED_DATA_USE')
// Data processing options
expect(fields.dataProcessingOptions?.properties?.limitedDataUse.type).toBe('boolean')
expect(fields.dataProcessingOptions?.properties?.otherDataProcessingOptions.type).toBe('string')
expect(fields.dataProcessingOptions?.properties?.otherDataProcessingOptions.multiple).toBe(true)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed, refer to my other comment for why

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