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

adding PAF userId and RTD modules. test paf bid adapter #46

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

bwschmidt
Copy link

Description of change

WIP paf modules

@@ -0,0 +1,102 @@
# Prebid Addreaability Framework (OneKey)

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({

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 ?

Copy link
Author

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

}
}
}

Choose a reason for hiding this comment

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

missing closing ```

identifiers: idsAndPreferences.identifiers,
preferences: idsAndPreferences.preferences
transmission: {
seed: seed

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
}

| 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 |

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.

Copy link
Author

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

while the pafIdSystem will pass the pafData.

Background information:
https://github.com/prebid/addressability-framework

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)

}

function setData(seed, rtdConfig, onDone) {
logMessage('DEBUG(seed):', seed);

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.

return mergeDeep(target, source);
}

function setData(seed, rtdConfig, onDone) {

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

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Author

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

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.

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?

Copy link
Author

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.

Copy link

@broggeri broggeri Apr 15, 2022

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?

@leonardlabat
Copy link

leonardlabat commented Apr 14, 2022

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.
It seems that you need to update eids.js source file to put a config function for 'pafData' eid kind.

I did the change below :

// paf
  'pafData': {
    source: 'paf',
    atype: 1,
    getValue: function (data) {
      return data.identifiers[0].value;
    },
    getUidExt: function (data) {
      return {
        version: data.identifiers[0].version,
        type: data.identifiers[0].type,
        source: data.identifiers[0].source
      };
    },
    getEidExt: function (data) {
      return {
        preferences: data.preferences
      };
    }
  },

And it successfully produces this kind of eids in my bid request :

[
  {
    "source": "paf",
    "uids": [
      {
        "id": "0c7966db-9e6a-4060-be81-824a9ce671d3",
        "atype": 1,
        "ext": {
          "version": "0.1",
          "type": "paf_browser_id",
          "source": {
            "domain": "crto-poc-1.onekey.network",
            "timestamp": 1649952764113,
            "signature": "cAudj1JsA2IqrOx8bt/1wiijLiAnsbALw+8c0h6Z8YrOIp/jsJoa5QHfvi+SL6wcorwwZifvfj0j0ERY3baiSg=="
          }
        }
      }
    ],
    "ext": {
      "preferences": {
        "version": "0.1",
        "data": {
          "use_browsing_for_personalization": true
        },
        "source": {
          "domain": "cmp.pafdemopublisher.com",
          "timestamp": 1649952764113,
          "signature": "e8daiMMRZISmyDTYhIwxOgi4eOedTDh5Ne3ACJ2JbUK0EzxZjAUn/ak7FOlEsPpfgw3WQGFKVYI5sz+ADqvGxA=="
        }
      }
    }
  }
]

(@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: {

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

Copy link
Author

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

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.

}

logMessage('DEBUG(idsAndPreferences):', idsAndPreferences);
window.PAF.createSeed({proxyHostName: rtdConfig.params.proxyHostName, callback: function (seed) {setData(seed, rtdConfig, onDone);}}, transaction_ids)

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)

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.

@bwschmidt
Copy link
Author

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

Copy link

@RomainLofaso RomainLofaso left a 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).

@bwschmidt bwschmidt force-pushed the paf branch 2 times, most recently from b860051 to 2790e58 Compare April 19, 2022 18:24
@bwschmidt
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants