Migrate Segment Amazon Conversion connector to CAPI v2 open beta API#3550
Migrate Segment Amazon Conversion connector to CAPI v2 open beta API#3550bansrav wants to merge 3 commits intosegmentio:mainfrom
Conversation
| required: false, | ||
| defaultObjectUI: 'keyvalue', | ||
| additionalProperties: true, | ||
| additionalProperties: false, |
There was a problem hiding this comment.
This change is intentional to limit the custom attributes to max 13 as Amazon API supports only 13 fields.
|
Hello @bansrav good to hear from you, and thanks for raising this PR! This PR introduces breaking changes, because there are new, required fields being defined. Renaming a field is the same as defining a new field. So we have a choice to make. Do we deploy this code over the code which is currently live, which will break data collection for customers who are already using the integration? Or do we register this code as a new integration, and leave customers on the old 'closed beta' version, and let them migrate over in their own time. It was a while since we last discussed this in person, so worth asking again. I'm OK with whichever approach you want to go for, but if we go for the first option we need to reach out to customers first. Kind regards, |
| * A list of flags for signaling how an event shall be processed. Events marked for limited data use will not be processed. | ||
| */ | ||
| dataProcessingOptions?: string[] | ||
| dataProcessingOptions?: string |
There was a problem hiding this comment.
Hi @bansrav - converting this to a string (from a string array) is likely to break things for customers who are already passing string arrays.
Is this type change absolutely needed?
There was a problem hiding this comment.
thanks for the comment. In the UI today, we have bounded value for this array which is choices: [{ label: 'Limited Data Use', value: 'LIMITED_DATA_USE' }],. Can customers pass multiple values in it? I didn't find a way to pass multiple values. Let me know if I am missing something.
In existing Amazon Conversions API spec, this field is modeled as below so even if any customer is passing multiple values today, the request validation on Amazon side will throw 400 error due to maxItem constraint.
dataProcessingOptions:
--
| | type: array
| | minItems: 0
| | maxItems: 1
| | items:
| | type: string
| | enum:
| | - LIMITED_DATA_USE
In the new CAPI v2 open beta spec, this field is modeled as object shown below which means there is no way to accept multiple values and if customer even pass multiple value, we have to choose the first one and pass it to Amazon API.
"dataProcessingOptions": {
"options": "LIMITED_DATA_USE"
},
If we keep this field modeled as array in UI, I feel that this gives bad customer experience as customers might complaint that we are passing multiple values, but on Amazon side, only one value is present so I thought to simplify this field mapping in UI.
Let me know your thoughts.
There was a problem hiding this comment.
Thanks for explaining @bansrav - that makes sense.
Do you know if this field is intended to evolve so that additional values could be passed? How likely is it that you might need to set more than 1 value at the same time for this field?
I'm not sure if converting the field from a string[] to string will cause a validation error in the pipeline (for existing customers). We should check this.
alternatively, we could delete this field and add a new optional field. Then in code you could default to LIMITED_DATA_USE if no value is provided. I think this might be the simplest approach.
There was a problem hiding this comment.
Do you know if this field is intended to evolve so that additional values could be passed? How likely is it that you might need to set more than 1 value at the same time for this field?
This field is not expected to evolve and the usages of this field will be very limited today. I don't think any advertiser uses it at all because the meaning of LIMITED_DATA_USE is to ignore the event on our side from processing. It's like advertiser send Amazon the event but ask to ignore it. I don't think any advertiser is using it today.
I'm not sure if converting the field from a string[] to string will cause a validation error in the pipeline (for existing customers). We should check this.
Not sure how I can validate it on my side. I have tested the changes in my local though.
alternatively, we could delete this field and add a new optional field. Then in code you could default to LIMITED_DATA_USE if no value is provided. I think this might be the simplest approach.
No, we shouldn't be creating new field and by default populate LIMITED_DATA_USE. As I described above, the meaning of this field is to completely ignore the event for processing.
I feel you are reluctant to change the data type from string[] to string for backward compatibility which makes sense and to fix it, I have reverted this change now in latest commit. Basically, we still accept as array but while mapping the payload to amazon API spec, I use the 0th index element.
Can you please review the latest code changes?
joe-ayoub-segment
left a comment
joe-ayoub-segment
left a comment
There was a problem hiding this comment.
Hi @bansrav please look at my comment regarding the dataProcessingOptions field being changed from string[] to string.
2 participants
A summary of your pull request, including the what change you're making and why.
Amazon Conversions API currently uses CAPI v2 closed beta end-point. We need to migrate the plug-in to CAPI v2 open beta end-point - see API Spec for CAPI v2 open beta. There is a difference in the end-point and the field mapping / names in closed beta and open beta API. In this PR, I am migrating the Segment connector to use open beta API which is our north star API for advertisers event ingestion.
Testing
Tested both sync and async/batch in local in postman. Here are the testing results:
Sync invocation
Segment API Request
Segment API Response
Async / Batch invocation
Segment API Request
Async / Batch API Response
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.
Security Review
Please ensure sensitive data is properly protected in your integration.
type: 'password'New Destination Checklist
verioning-info.tsfile. example