-
Notifications
You must be signed in to change notification settings - Fork 320
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
Issue-476 XDM schema definition for AdCloud Search surfer profiles #477
Issue-476 XDM schema definition for AdCloud Search surfer profiles #477
Conversation
I didn't get any response to my review comments on this one. It needs some work in terms of naming conventions. I am pushing this out of 0.9.5. |
@kstreeter I dont see any review comments here, Where can I find the comments ? |
"definitions": { | ||
"synchronized-search-retargeting-entity": { | ||
"properties": { | ||
"xdm:retargeting-entity-id": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
camelCase
3c997f9
to
57a03d6
Compare
…mplement review comments, follow camelCase for naming fields, refactor names
@kstreeter Kindly review the changes, I have implemented camelCasing for all fields. Also, I have refactored some field names. |
"xdm:status": "realized" | ||
} | ||
], | ||
"xdm:syncedremarketingaudiences": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this property defined? I think this is the profile schema, but I didn't see any changes to the ad cloud profile in the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, this should be camelCase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This property got missed somehow in the commit. It was supposed to be present in adcloud remarketing profile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
…mplement review comments-2, fix missing field for syncedRemarketingList in adcloud profile, update field description for partner data
@kstreeter Kindly review the changes. I have implemented the review comments. Thanks. |
Updated field "xdm:id" to "xdm:segmentID" to match the example and make it consistent with the same field in the segment schema
Updated field "xdm:id" to "xdm:segmentID" to match the example and make it consistent with the same field in the segment schema
…ud-Search-surfer-profiles
make linter happy
make linter happy
Hey @abhinnaagarwal-adobe, I made one fix to make the schema definition of segment ID match the example. Please let me know if this isn't what you intended. Otherwise this one looks ready to merge. |
@kstreeter Thanks for the approval. |
"definitions": { | ||
"partnerdata": { | ||
"properties": { | ||
"xdm:id": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cannot redefine the type of "xdm:id"
. It is already defined as a string https://github.com/adobe/xdm/blob/master/schemas/context/identity.schema.json
Please convert this to a string or define a different property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
converted to string
@@ -0,0 +1,4 @@ | |||
{ | |||
"xdm:id": 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment on the schema for this.
"xdm:timestamp": "2018-08-07T08:06:34.672Z", | ||
"xdm:partnerDetails": [ | ||
{ | ||
"xdm:id": 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment on the schema for this.
"xdm:timestamp": "2018-08-07T08:06:34.672Z", | ||
"xdm:partnerDetails": [ | ||
{ | ||
"xdm:id": 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment on the schema for this.
"$ref": | ||
"https://ns.adobe.com/experience/adcloud/syncedremarketingaudience" | ||
}, | ||
"description": "List of Search retargeting entities" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is description is not helping me understand the actual way to use this, can it be improved please?
"description": | ||
"Internal mapping of search/marketing platforms/partners to IDs for Adcloud Search." | ||
}, | ||
"xdm:data": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this some sort of packed/complex multi value data value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, We are planning to use it to store data such as conversion id/ conversion label that adCloud Search partner such as Google or Facebook will return. The conversion id is a unique identifier that is linked to a segment id. Also, we can use the same segment to create audiences on multiple partner platforms, hence this data-structure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have two concerns about this PR. I am not ready to approve.
- This seems like a straightforward audience assertion. Which we already have a
segments[]
property for. Why is this a new definition. Can you please review with @harleensahni to see if this can be integrated with the current definitions. - There is a redefinition of the type of
"xdm:id"
. It needs to be a string, or a different property.
…mplement review comments-3, Make partner id as string
@cdegroot-adobe I have implemented the review comments. Kindly review the changes. We want to store details about a retargeting entity supported by RLSA server. This is used to retarget surfers across different search platforms. The retargeting entity data (segment data) is synced to search engines, and RLSA server needs to keep a record on what has been synced to avoid resyncing. The search engine will return a unique identifier for the synced entity for eg conversion id label etc, which has to be stored in the surfer profile. Also, we can use the same retargeting entity to create audiences on multiple partner platforms. We could have used payload values in segment membership to store the data, if the payload values would support an arbitrary map where we can store all the data related to the segment |
"definitions": { | ||
"syncedremarketingaudience": { | ||
"properties": { | ||
"xdm:segmentID": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use SegmentIdentity here as the type?
{ | ||
"xdm:segmentId": "https://data.adobe.io/entities/aam-segment/4238827", | ||
"xdm:segmentType": 3, | ||
"xdm:timestamp": "2018-08-07T08:06:34.672Z", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This timestamp might be better in partner details to signify when the sync happened with the partner since it could have happened for one and not the other
"xdm:partnerDetails": [ | ||
{ | ||
"xdm:id": "2", | ||
"xdm:data": "sample-conversion-id-123" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the xdm:data seems like it should live elsewhere since this mapping between segment in AAM to external system like google for facebook is always the same from one AAM segment to one FB or Google Segment and divorced from the concept of when semgent was synced for an individual user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could structure this with partner details using segmemnt membership to store synced timestamp in the payload, but would need to repeat for each partner
We should find someplace to place the mapping from a segment in our systems (like AAM) to the segment id in another system like Google's for Facebooks. This feels like it belongs somewhere where se store segment definitions, and not segment memberships. The timestamps for when a segment for a user was synced with Google or Facebook seems like it could be a part of segment membership payload. |
Reviewed, comments left. Comments do not need to be addressed since I do not have adequate background on the project, and this is internal to AdCloud. I think if AdCloud can change this schema later without it being a breaking change for any other team (which should definitely be the case since it's there own extension -- if that's not a case that raises serious questions about the purpose of extension), it's fine to proceed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My clarifications have been answered. I approve, thank you team.
Hey all, I filed #520 to track Harleen's suggestions. We don't actually have a model for segment definitions in XDM yet, so we can't do this now but we should address when a segment model is created. |
…ud-Search-surfer-profiles
Please link to the issue #…
#476