Skip to content
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

Permutive RTD module: support IAB Audience taxonomy #8242

Merged
merged 11 commits into from
Apr 11, 2022

Conversation

desbo
Copy link
Contributor

@desbo desbo commented Mar 31, 2022

Type of change

  • Feature

Description of change

Updates the Permutive RTD module to facilitate for segmentation by
the new IAB Audience taxonomy.

To achieve this, this change introduces the concept of "transformations"
on the ORT2B user.data object. There are two components to these
transformations: a new transformations property on the Prebid config,
to be set by the publisher, and logic in the module for the actual
behaviour of the transformation.

We plan to use the transformation logic in this PR, combined with
configuration we'll share with publishers, to send IAB Audience
taxonomy cohort IDs to bidders.

Updates the Permutive RTD module to facilitate for segmentation by
the new IAB Audience taxonomy.

To achieve this, this change introduces the concept of "transformations"
on the ORT2B `user.data` object. There are two components to these
transformations: a new `transformations` property on the Prebid config,
to be set by the publisher, and logic in the module for the actual
behaviour of the transformation.

We plan to use the transformation logic in this PR, combined with
configuration we'll share with publishers, to send IAB Audience
taxonomy cohort IDs to bidders.
@lgtm-com
Copy link

lgtm-com bot commented Mar 31, 2022

This pull request introduces 1 alert when merging 000411a into 82987c6 - view on LGTM.com

new alerts:

  • 1 for Superfluous trailing arguments

@patmmccann
Copy link
Collaborator

patmmccann commented Apr 2, 2022

This feels like a super clunky experience for publishers. Why doesn't your endpoint return the transformation mappings instead of asking publishers to define potentially enormous config?

Also, it would be super trivial to take segtax as an input instead of hard coding it to '4'

@patmmccann patmmccann self-assigned this Apr 2, 2022
const ortb2UserDataTransformations = {
iabAudienceTaxonomy11: (userData, config) => ({
name: userData.name,
ext: { segtax: '4' },
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you're making pubs define all the transformations, no reason you can't let this be a param...

},
{
name: 'permutive.com',
ext: { segtax: '4' },
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's an integer

segment: expectedTargetingData
},
{
name: 'permutive.com',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it really permutive that decided the audience member belongs in that iab segment if permutive fails to provide the transformations? The publisher could easily transform any permutive segment to any iab segment; so I'm not sure this remains correct.

const segment = segmentData.ac.map(seg => {
return { id: seg }
})
function mergeOrtbConfig (currConfig, segmentIDs, transformationConfigs) {
Copy link
Collaborator

@patmmccann patmmccann Apr 2, 2022

Choose a reason for hiding this comment

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

There's a utils function for this now, i think it's called mergeDeep

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion – after looking at this function I realised it's not really doing a simple merge of two objects but actually updating the user.data array with some new entries. So I don't think mergeDeep is suitable here, but I've renamed the function to clarify what it does.

@desbo
Copy link
Contributor Author

desbo commented Apr 4, 2022

@patmmccann thanks for the review so far. In terms of the overhead for publishers: I should have mentioned that this change is to support a test with a single publisher that will only involve a small selection of cohorts. For this test, Permutive will provide the publisher with the segment mapping for use in their config, and we already have agreement from the publisher on this way of working for the test.

Our plan was to go with this approach for the test and then revisit our implementation so that the segment mappings come from our SDK rather than the Prebid module in the long term. Ultimately I'd expect that publishers would only need to include a simple flag in their config. Would you accept these changes to the config in light of this extra context? I'll address your other outstanding points of course.

@patmmccann
Copy link
Collaborator

patmmccann commented Apr 4, 2022

If you make segtax default to 4 but be overridable in config; i'd be happy to merge. Publishers want code that is "future-proof"; they don't want to have to suddenly upgrade prebid to support segtax i, j or k in their permutive adapter. We much prefer when adapters keep this in mind. If you look at the iab content taxonomy, it has quickly iterated from 2.1 -> 2.2 -> 3.0, each with a different segtax. I would not be surprised if audience gets a new segtax number shortly.

@desbo
Copy link
Contributor Author

desbo commented Apr 5, 2022

@patmmccann thanks! I've addressed your comments now, most importantly to include segtax as a config parameter.
Eventually we'll be back with another PR that removes the need for publishers to include the segment ID mappings in their config too, but hopefully this is acceptable in its current form for the test I mentioned.

edit to mention that I'll be away for the next couple of weeks and my colleagues @bartholomews and @Mullefa will commit to this branch if there are any more changes to make.

@desbo desbo requested a review from patmmccann April 6, 2022 09:29
name: 'www.dataprovider1.com',
ext: { taxonomyname: 'iab_audience_taxonomy' },
segment: [{ id: '687' }, { id: '123' }]
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

two things left: circleci needs to run; add a segtax here. Can be the permutive contextual segtax or the iab contextual (don't use the audience bc that is user.data)

@patmmccann
Copy link
Collaborator

@patmmccann thanks! I've addressed your comments now, most importantly to include segtax as a config parameter. Eventually we'll be back with another PR that removes the need for publishers to include the segment ID mappings in their config too, but hopefully this is acceptable in its current form for the test I mentioned.

edit to mention that I'll be away for the next couple of weeks and my colleagues @bartholomews and @Mullefa will commit to this branch if there are any more changes to make.

Thanks so much! Did you notice circleci didnt run? Usually helps to unfollow your fork in circleci before you make the change. Just one tiny change in the example file requested.

@desbo
Copy link
Contributor Author

desbo commented Apr 7, 2022

@patmmccann thanks again. I've added segtax to the example, hopefully in the right place but if not, please let me know and one of my teammates will fix. CircleCI is now running after updating this branch with master, although a lot of the tests are timing out – think these are unrelated to my changes?

@bartholomews
Copy link
Contributor

Hi @patmmccann circleci is now green, please let me know if there's anything else left to address! Thanks

@patmmccann patmmccann merged commit 930b94d into prebid:master Apr 11, 2022
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.

5 participants