-
Notifications
You must be signed in to change notification settings - Fork 2
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
[ACT-97] Support IAD SDA test in Permutive module #18
Conversation
346e61d
to
9f6f421
Compare
modules/permutiveRtdProvider.js
Outdated
@@ -236,11 +239,16 @@ export function getSegments (maxSegs) { | |||
ac: [..._pcrprs, ..._ppam, ...legacySegs], | |||
rubicon: readSegments('_prubicons'), | |||
appnexus: readSegments('_papns'), | |||
gam: readSegments('_pdfps') | |||
gam: readSegments('_pdfps'), | |||
ix: legacySegs |
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.
Currently this only includes standard cohorts (to be transformed to IAB IDs later) but I don't know if this is right. Maybe it should also include the cohorts from _ppam
and _pcrprs
(the same as ac
)?
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.
Some initial 'feedback' (mainly questions). I'm hoping I will be able to give a more insightful review in a second round one my understanding improves. But everything looks good so far!
modules/permutiveRtdProvider.js
Outdated
@@ -236,11 +239,16 @@ export function getSegments (maxSegs) { | |||
ac: [..._pcrprs, ..._ppam, ...legacySegs], | |||
rubicon: readSegments('_prubicons'), | |||
appnexus: readSegments('_papns'), | |||
gam: readSegments('_pdfps') | |||
gam: readSegments('_pdfps'), | |||
ix: legacySegs |
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.
(not introduced in this PR) @dreischer why did we called these segments legacySegs
? Would standardCohorts
not be a more indicative name?
@Mullefa thanks for the review so far, I've just found quite a major bug in that the transformations aren't being applied for index because I missed the fact that the I'm looking at this now but it makes me think that I haven't quite modelled it right and instead of thinking in terms of per bidder we should be thinking about different taxonomies that we want to support and how to build segment IDs appropriately for each one. |
8982b28
to
bed6be1
Compare
@Mullefa have applied the changes we discussed, think it's a real improvement 👍 |
modules/permutiveRtdProvider.md
Outdated
| params | Object | | - | | ||
| params.acBidders | String[] | An array of bidders which should receive AC cohorts. | `[]` | | ||
| params.maxSegs | Integer | Maximum number of cohorts to be included in either the `permutive` or `p_standard` key-value. | `500` | | ||
| params.transformations | Object[] | An array of configurations for ORTB2 user data transformations |
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.
Are the transformations intended to map standard cohorts to different IAB taxonomies, or something more generic?
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.
For the parameter I've modelled it very generally, as any transformation on the user.data
object without saying anything specific about what the transformation would do. WDYT?
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 think it's essential to document the different transformations available, so that people can effectively use the module.
My preference would be to have something less general:
{
"iabTaxonomies": {
"<taxonomyId>": {
"<standardCohort>": "<iabCohort>"
}
}
}
I think this would simplify the implementation, and support other IAB taxonomies out-of-the-box. It would also be easier to write documentation for.
That being said, main emphasis of this comment is +1 for documenting each transformation, rather than the configuration specifics (which is more just another approach for you to weigh 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 was thinking about it differently: I don't think it's guaranteed we're going to stick with this approach beyond the Hearst test (e.g. if we want to run this properly for all standard cohorts, I doubt we'll ask publishers to include the full list of cohorts in their config) and I'm expecting us to provide some specific documentation to Hearst (separately to this readme) that includes the config code they need to include as well as mentioning that this will apply to all bidders.
I totally agree that we should document the transformations we support but wonder if it's appropriate to add something that might only be accurate for the short duration of the Hearst test?
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.
Ok, but I'm not sure Prebid will see it this way. My guess is that they will want functionality merged into the SDK to be general and not tied to a specific use case. But happy to raise as is and not speculate.
That being said, if we don't want to document the IAB taxonomy transformation, what is the point of documenting transformations?
If we did want to the document the IAB taxonomy transformation, we could mark it as alpha in the docs and subject to change.
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 it's not that useful to have an entry on transformations when we don't say much about how to use it. But let's see what Prebid say.
maxSegs: 500, | ||
transformations: [ | ||
{ | ||
id: 'iabAudienceTaxonomy', |
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 don't think this is precise enough; there are multiple IAB taxonomies
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.
only 1 Audience taxonomy I think?
https://iabtechlab.com/standards/
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.
Sorry for not being clear -- I was thinking perhaps a user of the module should be able to configure the version used?
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.
Yep, fair enough
modules/permutiveRtdProvider.md
Outdated
| params | Object | | - | | ||
| params.acBidders | String[] | An array of bidders which should receive AC cohorts. | `[]` | | ||
| params.maxSegs | Integer | Maximum number of cohorts to be included in either the `permutive` or `p_standard` key-value. | `500` | | ||
| params.transformations | Object[] | An array of configurations for ORTB2 user data transformations |
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 think it's essential to document the different transformations available, so that people can effectively use the module.
My preference would be to have something less general:
{
"iabTaxonomies": {
"<taxonomyId>": {
"<standardCohort>": "<iabCohort>"
}
}
}
I think this would simplify the implementation, and support other IAB taxonomies out-of-the-box. It would also be easier to write documentation for.
That being said, main emphasis of this comment is +1 for documenting each transformation, rather than the configuration specifics (which is more just another approach for you to weigh 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.
Let's see what people far more qualified than me say 😅
f3d166c
to
389b7fe
Compare
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.
389b7fe
to
000411a
Compare
* pageURL pull from topmostLocation * Kargo: Support for client hints (#9) * Starting SUA support * Kargo: Adding support for client hints * Adding tests for sua * Kargo: Update referer logic * Refactor of Kargo Prebid adapter. * PR comments addressed. * Feedback addressed. * Pr comments addressed. * Continuing refactor of Kargo Bid adapter. * Logic adjustment to exclude values when not present. Relying on server defaults. * Updating unit tests. * PR feedback addressed. * Refactoring bid adapter functions. * PR feedback addressed. * Additional refactoring. * Refactoring for each to use Object entries. * Minor fixes. * Minor fixes. * Minor fixes. * TDID and linting updates * Conflicts resolved with master. * Re-adding raw CRB storage (#14) * Updating shared IDs object name * Fixing missing ad markup * Removing package json changes. Fixing unit tests broken by recent changes. * Linting * send requestCount even when it is 0 for BTO (#18) * Reverting package.json change * Reverting package-lock.json changes * Cleanup * Test cleanup * Test fix Test fix All tests fixed * Adding test for TDID * Resolving merge issue --------- Co-authored-by: Neil Flynn <nflynn@kargo.com> Co-authored-by: Julian Gan <juliangan07@gmail.com>
* create setupadBidAdapter * add setupadBidAdapter * update setupadBidAdapter * update metrics collection * update analytics collection * update getUserSyncs * add setupadAnalyticsAdapter.js * test setupadAnalyticsAdapter * remove test: 1 * add GVLID && bug fixes && test updates * remove setupadAnalyticsAdapter * add userID module handling * add GVLID && bug fixes && test updates * remove setupadAnalyticsAdapter * add userID module handling * clean up && seat bugfix * clean up logs * add userID module handling * update md && clean up * Send setupad only on bidRequested * Fix bidResponse and bidWon responses * Improve bidResponse and bidWon logic * Revert changes to specific files * Remove test parameter * Fix multiple bidResponse and bidTimeout calls to getPixelUrl * eslint errors fixes(brackets added) * Add extra checks for events * Fix BIDDER_CODE const * update reporting endpoint * update setupadBidAdapter_spec.js REPORT_ENDPOINT * update readme * Revert "Merge branch 'prebid:master' into setupad-adapter" This reverts commit 1c14dbe, reversing changes made to 7fe9ea5. * Revert "Revert "Merge branch 'prebid:master' into setupad-adapter"" This reverts commit a34e3e4. * # This is a combination of 20 commits. # This is the 1st commit message: add setupadBidAdapter # This is the commit message #2: update setupadBidAdapter # This is the commit message #3: update metrics collection # This is the commit message #4: update analytics collection # This is the commit message #5: update getUserSyncs # This is the commit message #6: add setupadAnalyticsAdapter.js # This is the commit message #7: test setupadAnalyticsAdapter # This is the commit message #8: remove test: 1 # This is the commit message #9: add GVLID && bug fixes && test updates # This is the commit message #10: remove setupadAnalyticsAdapter # This is the commit message #11: add userID module handling # This is the commit message #12: clean up && seat bugfix # This is the commit message #13: add userID module handling # This is the commit message #14: add GVLID && bug fixes && test updates # This is the commit message #15: remove setupadAnalyticsAdapter # This is the commit message #16: add userID module handling # This is the commit message #17: clean up logs # This is the commit message #18: update md && clean up # This is the commit message #19: Send setupad only on bidRequested # This is the commit message #20: Fix bidResponse and bidWon responses * # This is a combination of 22 commits.tree 8abae7e6dffc9a21ad11770713ba485fc610028a parent cecfce3 author pavel <pavel@setupad.com> 1706627437 +0200 committer pavel <pavel@setupad.com> 1706627437 +0200 gpgsig -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEqGYI3KX/FkbObQG8FABtd4pCs/AFAmW5EW0ACgkQFABtd4pC s/CK3w//WWJSFUlycnnNKTV2XfdcBjooOeZZvjpXVthwr09CCC4uO//kw4bPluhn f5fcVFdXzrY1AZ6ch8Wo3msX/Pkso014jIGd5aIWcHpNYFtffACwH/40Y8AcJNZd bsOZxVK0awPTz/RihC5eY+0J3cP+iFWP/FlYJoHEQIBXq/Eg6mWoAhxwpL/JvxbY QbLFWsRn2ckQ6ftOZgm3/jh8VLaG1zWbWImlWEs5Zel+CorJBTniTj58VbApelYD TFMgbSR2I4NGVaqNIrHePnSMsDATxalQ2nZPwY6raKCHWIbvoUPIn/OpDMMbKgC7 nCwounNmObxFVoj3xusAZppzHpKPasY8xKWb2Kr7zfhZArsOMC6B7fYqQNK0cWG3 8RR/10oheJD9M2kRlfLiqnRv7ExY08SQ/ZMo9LA8BeRUGBXhh6++8FKhKIHvX1gL k1R5W6c+NNWP+PDFsmrFpMn+LpYdl84I7yfYK5dHuw80od7f1wuAVYpswi6Cziy9 /KY6/rfENvUrGTmWSh5GdDBel89ACCfFkasIKB92xhzKTfjzF/DXkc8XQZOMbt1j CsILgWMNfLPMo4Dlgdx/tYCSLLBNEtZ1/hhUcFQ3+0TzLf0GtMkvMnlBnDinqe1n 1P30fQ2I5W5NJKDPrCOnRymI6QOAPFXtMF11R81mbB9H8asft/E= =oJtZ -----END PGP SIGNATURE----- bugfixes # This is the commit message #22: Remove test parameter * # This is a combination of 26 commits. parent cecfce3 author pavel <pavel@setupad.com> 1706627437 +0200 committer pavel <pavel@setupad.com> 1706627437 +0200 gpgsig -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEqGYI3KX/FkbObQG8FABtd4pCs/AFAmW5EW0ACgkQFABtd4pC s/CK3w//WWJSFUlycnnNKTV2XfdcBjooOeZZvjpXVthwr09CCC4uO//kw4bPluhn f5fcVFdXzrY1AZ6ch8Wo3msX/Pkso014jIGd5aIWcHpNYFtffACwH/40Y8AcJNZd bsOZxVK0awPTz/RihC5eY+0J3cP+iFWP/FlYJoHEQIBXq/Eg6mWoAhxwpL/JvxbY QbLFWsRn2ckQ6ftOZgm3/jh8VLaG1zWbWImlWEs5Zel+CorJBTniTj58VbApelYD TFMgbSR2I4NGVaqNIrHePnSMsDATxalQ2nZPwY6raKCHWIbvoUPIn/OpDMMbKgC7 nCwounNmObxFVoj3xusAZppzHpKPasY8xKWb2Kr7zfhZArsOMC6B7fYqQNK0cWG3 8RR/10oheJD9M2kRlfLiqnRv7ExY08SQ/ZMo9LA8BeRUGBXhh6++8FKhKIHvX1gL k1R5W6c+NNWP+PDFsmrFpMn+LpYdl84I7yfYK5dHuw80od7f1wuAVYpswi6Cziy9 /KY6/rfENvUrGTmWSh5GdDBel89ACCfFkasIKB92xhzKTfjzF/DXkc8XQZOMbt1j CsILgWMNfLPMo4Dlgdx/tYCSLLBNEtZ1/hhUcFQ3+0TzLf0GtMkvMnlBnDinqe1n 1P30fQ2I5W5NJKDPrCOnRymI6QOAPFXtMF11R81mbB9H8asft/E= =oJtZ -----END PGP SIGNATURE----- bugfixes # This is the commit message #22: Remove test parameter # This is the commit message #23: Fix multiple bidResponse and bidTimeout calls to getPixelUrl # This is the commit message #25: eslint errors fixes(brackets added) # This is the commit message #26: Add extra checks for events * parent 75178b9 author pavel <pavel@setupad.com> 1706627694 +0200 committer pavel <pavel@setupad.com> 1706627694 +0200 gpgsig -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEqGYI3KX/FkbObQG8FABtd4pCs/AFAmW5Em4ACgkQFABtd4pC s/BBUQ/+NXyHoxPM185YJLG9M1ySC/5vTT9W5mfwQ93cVDLCeuGnpsnmi4S21NuQ b7gSeokFjwztvVOUmh/xqMp4lTsvL53TUd00b1k4KGVSqgcF00Foit5g8fOGLYsI DAoqphYV6MWjpAun+II+ELY8QUkHR1cjTc7PEGtmf+8RnptGVdyJ6C9Ab8u9TQTY Apj5Srhfo3Tl8S+WScOxwwB/uqEJR4fhIrJyzFzdLDEb2olSPyrQUs87vQXlhEnK buPEg2F5JsRH6sw11Xp3TFNSZGxNnBSlTh9dixou5md4yRCv5a2TMef667N0BVDp lGgc7mCrRKXyqzphmmeHudiscEGFjtUPObXoHutSVw22wdARFCTpNFKBLLFn4v8o Zv1OvFdNprvHsoeW0HVlZdU7OKnDTRrko6DHk2AahxojjvAFEWuDsGYZNjhdQwRR lK1zm+SFQnKI0Eojd+f84fvKod9geGs640jyH/x5R4eYm4yjZb8SkRtd3cca88wS OuGq9LIkbU428b46l7VnDwudldTXPUU8eKfUtFRjdGtIWH9I3tK6TsRoCfTcXkv0 smxYiiU1XHjAkkPFWQWEeFdfZ071snFKVWouU0AoKiq+PdRoS8+3AJqIQUjlA2sH AybnSkv9KxY/Rs1bnvMubsQm1GF66qVrbxBU6FILBv1JZYwj4yA= =Gbog -----END PGP SIGNATURE----- bugfixes update setupadBidAdapter_spec.js REPORT_ENDPOINT update readme Revert "Merge branch 'prebid:master' into setupad-adapter" This reverts commit 1c14dbe, reversing changes made to 7fe9ea5. Revert "Revert "Merge branch 'prebid:master' into setupad-adapter"" This reverts commit a34e3e4. * change double quote to single quote --------- Co-authored-by: pavel <pavel@setupad.com> Co-authored-by: Elgars Grodnis <elgars@setupad.com> * bugfix setupadBidAdapter remove getAdEl, spelling correction * add onBidWon event onBidWon event handling moved from custom to native onBidWon method * minor bugfixes && remove funk getSiteObj && getDeviceObj --------- Co-authored-by: pavel <pavel@setupad.com> Co-authored-by: Elgars Grodnis <elgars@setupad.com>
Updates the Permutive RTD module to support a test for segmentation with
the new IAB standard data taxonomy.
Initially we are mapping a small selection of Permutive to IAB cohort
IDs and including these in the ORTB2 object. We also set the
segtax
value appropriately for the Index exchange, which we're using for this
test.
Type of change
Description of change
Be sure to test the integration with your adserver using the Hello World sample page.
For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide:
Other information