Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 submodule #6290
Permutive RTD submodule #6290
Changes from 4 commits
a13ee4c
593f151
98463f0
41e532f
19d5f4f
3cf5560
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
FWIW - while this is ok for Feb 10, it hoped/expected that most adapters will soon be able to start reading ortb2.user.data to get segments. When rubicon supports this, will want to update the example.
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.
Good shout - please see comment below
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.
Please rename this function to something like "initSegments". Prebid RTD modules uses the word "targeting" to mean ad server targeting -- as a reviewer, I find it confusing to sort out the terminology.
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.
Please also consider conforming to the new segment syntax defined by the Prebid Taxonomy committee and the IAB. ee #6057
I understand that right now this is a draft spec not implemented by the adapters, but... Chicken and Egg. If you start writing the segments now, the bidders can start picking them up when they're ready.
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.
Totally agree. We went for parity on how segments are passed into bidders as custom params today (today our clients add custom code to their sites, that sets bidder config as params). But we'd also prefer to use the proper FPD object instead. The issues we came across right now is that bidders do not accept and parse the new structure yet.
In the example below data isn't pared as expected:
results in:
Based on you other comment, I assume the idea is that we'd use
fpd.user.ortb2.data
instead offpd.user.data
? So set segments like below:What we could do is keep the current structure to make sure targeting works in the here and now, but then also set
fpd.user.ortb2.data
in parallel for when adapters accept that structure. Is this what you recommend? There are still a few open questions on our end to decide how to expose the rightname
andtaxonomyname
and what these values should be for different use-cases. If you're happy with it, I'd suggest to go ahead with v1 which takes the same approach as the current setup and we'll then follow up with a PR for v2 which adds support forfpd.user.ortb2.data
- ready for when individual adapters start picking it up.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 agree that the adapters aren't as of yet accepting user.data.segment. But I know that conversation is happening right now in the Magnite(rubicon) exchange team, so it won't be long before we can update the rubicon adapter to pass in the segments where they want them.
So for now, I'm suggesting pass the data BOTH ways: as bidder.params.visitor and as ortb2.user.data. Bidders will ignore the latter for now, but it being there from the start kickstarts the chicken/egg problem.
For the record, 'fpd' setConfig will be gone once #6293 is merged. So I recommend ignoring 'fpd' starting now -- I recommend doing a "getConfig" on ortb2, adding your stuff to the user.data array, and setConfiging it back.
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.
That makes perfect sense, thanks @bretg. Is it ok if we follow up with a separate PR for this? Implementing the change should be quick on our end. But we'll need to first understand what the appropriate values for
name
andtaxonomyname
are for the different use-cases we'll support with this module. Great to hear that the Magnite team is already working on supportinguser.data.segment
!Also, thanks for your note on
getConfig
before callingsetConfiging
- we want to make sure we're not overwriting any other segment data objects