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

bwschmidt · 2022-04-05T05:06:17Z

Description of change

WIP paf modules

broggeri · 2022-04-12T11:28:28Z

modules/pafIdSystem.md

broggeri · 2022-04-12T11:31:57Z

modules/pafIdSystem.md

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 ?

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

OlivierChirouze · 2022-04-14T13:20:48Z

modules/pafRtdProvider.md

missing closing ```

OlivierChirouze · 2022-04-14T13:25:40Z

modules/pafRtdProvider.js

[NIT] could be just

transmission: { seed }

OlivierChirouze · 2022-04-14T13:26:51Z

modules/pafRtdProvider.md

This is not a URL but just a servername. Maybe you could provide an example as well to avoid confusion.

changed it to servername, the example is above the table

OlivierChirouze · 2022-04-14T13:41:11Z

modules/pafRtdProvider.md

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

RomainLofaso · 2022-04-14T14:10:32Z

modules/pafRtdProvider.js

Check if the seed is undefined or null and if so call onDone earlier.

RomainLofaso · 2022-04-14T15:26:02Z

modules/pafRtdProvider.js

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

that is being set on line 32 https://github.com/openx/Prebid.js/pull/46/files#diff-82f56fea551670affb017e76a242b6899257dab99443ed3222ae5297161aa2e2R32

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

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

RomainLofaso · 2022-04-14T15:36:48Z

modules/pafRtdProvider.md

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?

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.

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 · 2022-04-14T16:20:09Z

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.

leonardlabat · 2022-04-14T16:36:12Z

modules/pafRtdProvider.js

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

i omit it because i believe it is set inside the seed, can copy it here if needed

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.

leonardlabat · 2022-04-14T16:37:38Z

modules/pafRtdProvider.js

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

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 · 2022-04-14T19:09:01Z

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

RomainLofaso

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 · 2022-04-20T03:15:55Z

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

* AGT685: Remove device info logic (#42) * AGT-680: Don't send gam in browserBlacklist (#44) * AGT-680: Dont send gam in browserBlacklist * AGT-680: fix commas * Agt 682 implementation for module (#45) * add GAM prediction * Update version number, add tests, update documentation * Fix property name * refactor export file * fix import typo * remove GAM prediction flag * extend GAM prediction documentation * fix configuration name in documentation * Agt 698 module config changes (#46) * move analytic related params to analytic adapter * Move back Maintainer section --------- Co-authored-by: dmytro-po <dmytro.po@intentiq.com>

bwschmidt mentioned this pull request Apr 5, 2022

adding PAF userId and RTD modules to prebid OneKey-Network/OneKey-implementation#88

Merged

broggeri reviewed Apr 12, 2022

View reviewed changes

modules/pafIdSystem.md Outdated

Copy link

broggeri Apr 12, 2022

Choose a reason for hiding this comment

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

typo

broggeri reviewed Apr 12, 2022

View reviewed changes

OlivierChirouze reviewed Apr 14, 2022

View reviewed changes

RomainLofaso reviewed Apr 14, 2022

View reviewed changes

leonardlabat reviewed Apr 14, 2022

View reviewed changes

RomainLofaso approved these changes Apr 19, 2022

View reviewed changes

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

bwschmidt force-pushed the paf branch from e80c825 to 825b08b Compare May 9, 2022 19:52

bwschmidt added 6 commits May 11, 2022 12:43

adding PAF userId and RTD modules. test paf bid adapter

4d6b3db

add PAF readmes and small fixes

bd63aa4

address PR comments

95a704b

revert to OpenX serving test ads

fb62677

update pafRtdProvider to use generateSeed

ba04893

add PAF unit tests

dcb2e4f

bwschmidt force-pushed the paf branch from 825b08b to dcb2e4f Compare May 11, 2022 19:43

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

bwschmidt commented Apr 5, 2022

Description of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

broggeri Apr 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leonardlabat commented Apr 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwschmidt commented Apr 14, 2022

Uh oh!

RomainLofaso left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwschmidt commented Apr 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

broggeri Apr 15, 2022 •

edited

Loading

leonardlabat commented Apr 14, 2022 •

edited

Loading

RomainLofaso left a comment •

edited

Loading