-
Notifications
You must be signed in to change notification settings - Fork 5
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
adding PAF userId and RTD modules. test paf bid adapter #46
base: master
Are you sure you want to change the base?
Conversation
modules/pafIdSystem.md
Outdated
@@ -0,0 +1,102 @@ | |||
# Prebid Addreaability Framework (OneKey) |
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.
typo
information to bidders. Here is a configuration example: | ||
|
||
```javascript | ||
pbjs.setConfig({ |
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.
Question: I'm not very familar with prebid conf. Is it correct to assume that the paf id module will still be run even if no config is set ?
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 module has to be in the userIds array for it to be run even if its included in the build, although no configuration parameters are required
} | ||
} | ||
} | ||
|
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.
missing closing ```
modules/pafRtdProvider.js
Outdated
identifiers: idsAndPreferences.identifiers, | ||
preferences: idsAndPreferences.preferences | ||
transmission: { | ||
seed: seed |
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.
[NIT] could be just
transmission: {
seed
}
modules/pafRtdProvider.md
Outdated
| name | String | Real time data module name | Always 'paf' | | ||
| waitForIt | Boolean | Required to ensure that the auction is delayed until prefetch is complete | Optional. Defaults to false | | ||
| params | Object | | | | ||
| params.proxyHostName | String | URL of the PAF Proxy which will generate seeds. | Required | |
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 not a URL but just a servername. Maybe you could provide an example as well to avoid confusion.
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.
changed it to servername, the example is above the table
modules/pafRtdProvider.md
Outdated
while the pafIdSystem will pass the pafData. | ||
|
||
Background information: | ||
https://github.com/prebid/addressability-framework |
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.
[NIT] this is hardly readable on the "rendered" document. Consider something like:
- [prebid/addressability-framework](https://github.com/prebid/addressability-framework)
- [prebid/paf-mvp-implementation](https://github.com/prebid/paf-mvp-implementation)
modules/pafRtdProvider.js
Outdated
} | ||
|
||
function setData(seed, rtdConfig, onDone) { | ||
logMessage('DEBUG(seed):', seed); |
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.
Check if the seed is undefined or null and if so call onDone
earlier.
modules/pafRtdProvider.js
Outdated
return mergeDeep(target, source); | ||
} | ||
|
||
function setData(seed, rtdConfig, onDone) { |
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 catch yet the exactly what is going on in the positive branch of if (rtdConfig.params && rtdConfig.params.bidders)
. However, I don't see a direct association between one Transmission-Id and an AdUnit here and I think that it should be in this branch of code, isn't it? We need a transaction-id at the imp
level. See the following imp
object.
Example:
{
"id": "1",
"bidfloor": 0.03,
"banner": {
"h": 250,
"w": 300,
"pos": 0
},
"ext": {
"paf": {
"transaction-id": "4640dc9f-385f-4e02-a0e5-abbf241af94d"
}
}
}
See the example in this spec.
https://github.com/prebid/addressability-framework/blob/main/mvp-spec/dsp-implementation.md
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.
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, @RomainLofaso one note is that RTD modules can only set the impt.ext.data field, which is why the module uses ortb2Imp.ext.data.paf.transaction_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.
Ok, thank you for the clarification!
I don't think it is a big deal. So it would mean that we need to update the documentation
can quickly and easily setup the Prebid Addressability Framework and utilize OneKey. | ||
This module is used along with the pafIdSysytem to pass PAF data to your partners. | ||
Both modules are required. This module will pass transmission requests to your partners | ||
while the pafIdSystem will pass the pafData. |
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 it important to mention that the publisher and the owner of the adapter must have a mutual agreement on the model terms so that they can use this RTD module? Or is there a general rule for the adapters?
Linked discussion: Does the publisher can configure PrebidJs to activate the module for specific adapters and not for others?
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 i would like some help with these descriptions, i am not sure what the final model terms are and what we need to say.
The publisher can explicitly configure prebid.js to activate userId modules for specific adapters, but not for RTD modules, which is why this module allows that functionality through params.bidders.
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.
re model terms: I wouldn't refer to them as of yet as I still could read a recent version, but yes the idea is that the publisher should only send the PAF transmission to the relevant bidders.
I would suggest to add "Unless all your partners are PAF partners, you should whitelist the ones who are using param.bidders". (Either here, and/or in the bidders param description)
(Maybe crazy) thought: could the RTD module check the id module configuration to autoconfigure instead of taking a parameter ? Or is that level of introspection not possible or authorized?
Hello @bwschmidt While running a test of the pafidModule on a sample publisher page, I found out that the eids are not retrieved by the bid adapters. I did the change below :
And it successfully produces this kind of eids in my bid request :
(@OlivierChirouze @RomainLofaso let me know if this indeed looks like a valid PAF eids, I based myself on https://github.com/prebid/addressability-framework/blob/main/mvp-spec/dsp-implementation.md) The only culprit of my change is that it works only if PAF.getIdsAndPreferences returns a single identifier, but I feel that the current implementation of the way getValue/getUidExt are called doesn't allow an id module to produce several uids. |
user: { | ||
ext: { | ||
paf: { | ||
transmission: { |
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.
According to https://github.com/prebid/addressability-framework/blob/main/mvp-spec/dsp-implementation.md
There should be a "version" property under transmission, it it ok not to set it there ? @OlivierChirouze @RomainLofaso
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 omit it because i believe it is set inside the seed, can copy it here if needed
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.
Thanks, @leonardlabat .
There are a few elements that can be removed from the first transmission request like the receiver, the status...
However, I think that the version is needed and Prebid should set its own. Consider the case where the Seed is generated by the publisher that is in Vx and the Prebidjs is in Vx+1.
modules/pafRtdProvider.js
Outdated
} | ||
|
||
logMessage('DEBUG(idsAndPreferences):', idsAndPreferences); | ||
window.PAF.createSeed({proxyHostName: rtdConfig.params.proxyHostName, callback: function (seed) {setData(seed, rtdConfig, onDone);}}, transaction_ids) |
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.
@OlivierChirouze @RomainLofaso
I'm not an expert but I see that createSeed is using promise here https://github.com/prebid/paf-mvp-implementation/blob/ae8782f16f2aae49f79feb061ab67d0c0958e02a/paf-mvp-frontend/src/lib/paf-lib.ts#L454
Is it ok to call it this way ? (with a callback packed together in an object with the first argument, proxyHostName)
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.
Thanks for the heads up.
It is related to another PR on PAF: https://github.com/prebid/paf-mvp-implementation/pull/88/files/399fa630167948020d5b0b226da305713cfb3d47#diff-77c3a78d4eeb5e57d6a99735308b4e8982ca2de5077ac0755e8a619e4b2ed81cR137
The suggested change includes introducing this new callback
member on the configuration object.
Now, it is true that createSeed
itself is async
but it just means that this getBidRequestData
function will not wait for createSeed
to complete before to return, which I guess is ok.
ah yes, @leonardlabat thank you, i had made almost the exact same change to eids.js but forgot to add the file, i will update |
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.
If I understand well, all the necessary elements to make it works are here. Even if there are remaining things like adding the version of the Transmission, removing the host URL in the configuration and refining the documentation, I think that we have more to win by merging the PR in the current state quickly to unlock other integration (e.g Criteo one).
b860051
to
2790e58
Compare
thanks @RomainLofaso, likely wont ever merge this here, the goal is to just agree on the implementation and i will submit it to prebid core for merge, i will complete unit tests this week and hopefully update with better descriptions |
Description of change
WIP paf modules