Permutive RTD submodule: TrustX update #7001

dreischer · 2021-06-10T11:34:10Z

Update to how Permutive passes segments to the TrustX adapter

Type of change

Other

patmmccann · 2021-06-10T11:39:18Z

Can you start setting the global ortb2 object? All these custom sets are not ideal, as you point out

#6290 (comment)

There appears to be an agreement this is coming soon in the original review

#6290 (review)

Fwiw we have held other rtd submodules until they meet this requirement, eg digital Garage and sirdata.

dreischer · 2021-06-10T12:22:58Z

Can you start setting the global ortb2 object? All these custom sets are not ideal, as you point out

#6290 (comment)

Thanks for picking this up @patmmccann

TrustX have asked us to provide the data in specifically this format - @PWyrembak please let me know if you'd like us to stick with the structure in this PR, or to change it to something like

pbjs.setBidderConfig({
  bidders: ['trustX'],
  config: {
    ortb2: {
      user: {
        data: [{
          name: 'permutive.com',
          segment: [
            { id: '1' }
          ]
        }]
      }
    }
  }
})

(or we could do both in parallel)

patmmccann · 2021-06-10T19:33:03Z

I have no worries with you setting things in a specific way for any specific adapter as long as you also set global. We don't want RTD submodules becoming kingmakers of which bid adapters have access to seller defined audiences and which do not.

dreischer · 2021-06-11T12:03:57Z

@patmmccann - We want to give publishers the control of where to send specific segments to. For example, let's say a publisher creates a custom segment for a specific campaign and they're transacting with the buyer via a PMP in Xandr. If they only enable the Xandr activation for this segment in Permutive, we won't push the segment ID to any other SSP. This means (1) we define the key-value <> segment name mapping only in Xandr and (2) we push the respective segment ID only into the Xandr request on page.

It's similar for this PR: The publisher will need to explicitly instruct us to send a certain set of segments to TrustX. And these segment IDs wouldn't be of much value to other bidders, because they are created for specific use-cases and they're arbitrary IDs.

I'm very supportive of using the config.ortb2.user.data as a proper field for segments, instead of using key-values. The blocker right now is that ortb2 isn't supported by the SSPs we integrate with (both in Prebid and to use it for deal targeting) or that only specific segment taxonomies are supported. Regarding your point on making segments available globally: We have the requirements from publishers to send specific segments to specific SSPs. I don't believe that our clients necessarily want to share their segments with all bidders. So when using config.ortb2.user.data we will most likely do it via setBidderConfig so we can respect the publisher configuration. I don't want to sidetrack this PR too much - but if anything is unclear I'd be happy to continue the conversation via email or an issue on GitHub.

Fawke · 2021-06-15T09:55:04Z

The changes made in this PR looks good. But, I'd wait for @bretg to review as well. If I'm not wrong, I think he wrote a comment in one of your earlier PRs to set the data to both key/value pair and the ortb2 object.

bretg · 2021-06-17T17:23:25Z

It's ok if the bid adapter don't look at the standard location -- you can set it there anyhow. This solves the chicken/egg problem. Hopefully we can guilt them over time to getting it from the right place.

Here's what we want to see:

update your code so that when you set bidder-specific params, you also call pbjs.setBidderConfig() for that specific adapter like what's note above.
Update your documentation .md file to reflect this
Add a docs file in https://github.com/prebid/prebid.github.io/tree/master/dev-docs/modules/permutiveRtdProvider.md similar to other RTD module docs in that folder

Please work with Steve Francolla to determine the right syntax for the user.data[]. Permutive will need a segtax value -- Steve's the chair of the Prebid Taxonomy committee so should be able to make this happen.

This all said, if you are on a tight timeline for TrustX, we could accept a promise to make this change in a near-future release.

PWyrembak · 2021-06-18T15:36:39Z

Hi @dreischer @bretg
We just made this PR #7055 that add support for TrustX adapter to read Permutive segments from ortb2.user.data. From our POV this object works the best for this purpose so wanted to make sure we are aligned on both PRs.

Fawke

LGTM.

…e-trustx-update

patmmccann · 2021-06-22T12:23:40Z

#7074 appears to be an example of the poor behavior permutive is encouraging. I would suggest we hold this pr, 7074, and any other customizations related to permutive until they are in compliance.

We don't want every bidder having to make a pr to support permutuve and another to support ortb2 object.

dreischer · 2021-06-22T13:29:47Z

@PWyrembak that's great thanks. I've update the integration so we now use ortb2.user.data. It seems to work as expected.

@bretg Does this update look good to you? I had two questions:

Is there a better way to get the bidder config first, so we can merge into it? I'm worried that right now we're overwriting, but maybe .setBidderConfig() does a merge in the background anyway?
I had to remove the TrustX test, as we don't modify the bid object anymore. And I'm not sure there's another way to check since there's no .getBidderConfig() function. Any advice?

I'd be happy to set data for the other bidders the same way if that's preferred. As far as I'm aware the Magnite bidder will just filter it out and the Ozone and Xandr don't support it, but I guess it doesn't hurt if it's there. My only concern is how we can cover it with tests while it's not supported, but that is wider question anyway (question 2 above)

Thanks for the reminder for the dev-docs page @bretg - I'll make sure we get a PR for that

dreischer · 2021-06-22T13:42:15Z

@patmmccann I wasn't familiar with the PR you linked. But after taking a quick look it seems as if the PR is adding support for ortb2.user.data. Today the Permutive submodule does not connect to Pubmatic. But if their adapter now supports ortb2.user.data and they're also adding Pubmatic to the Permutive submodule, then I agree that we should use the same pattern as my last commit on this PR (pending @bretg's feedback). So it would look something like this:

pubmatic: function (bid, data, acEnabled) {
  if (acEnabled && data.ac && data.ac.length) {
    setBidderRtb('pubmatic', data.ac)
  }
}

patmmccann · 2021-06-22T13:47:25Z

Your last commit appears to only set the global if someone is using trustx. You should set the global regardless; you don't get to pick and choose which bidders have access to these data, publishers do.

…

On Tue, Jun 22, 2021, 7:42 AM David Reischer ***@***.***> wrote: @patmmccann <https://github.com/patmmccann> I wasn't familiar with the PR you linked. But after taking a quick look it seems as if the PR is adding support for ortb2.user.data. Today the Permutive submodule does not connect to Pubmatic. But if their adapter now supports ortb2.user.data and they're also adding Pubmatic to the Permutive submodule, then I agree that we should use the same pattern as my last commit on this PR (pending @bretg <https://github.com/bretg>'s feedback). So it would look something like this: pubmatic: function (bid, data, acEnabled) { if (acEnabled && data.ac && data.ac.length) { setBidderRtb('pubmatic', data.ac) }} — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7001 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAM25Z2S5XB2WQOV723S3ETTUCHMPANCNFSM46OKT5OQ> .

bretg · 2021-06-22T19:22:29Z

@dreischer - the requirements are:

No RTD module can favor a particular bidder.
All modules must support standard conventions for FPD, video, schain, coppa, etc.

That said, if you have to do special logic for certain bidders, not ideal, but it's ok as long as it's publisher-configurable and standard conventions are followed in addition to whatever special stuff is needed.

In your case:

if a bidder is defined in acBidders
write the data to the special place AND to user.data for that bidder.
if the publisher adds a bidder to the acBidders list that your module doesn't recognize, you still have to add an element to user.data for that bidder

You have two choices for writing to user.data on a per-bidder basis:

use getBidderConfig() to find the current custom config for a given bidder
a) merge a new ortb2.user.data object
b) write it back with setBidderConfig()
c) then Patrick and I start yelling at the bidders instead of you
help us improve the RTD module infrastructure by supporting the bidder-specific use-case rather than having to use setBidderConfig.
a) This could be done by adding a "bidRequest" event listener to https://docs.prebid.org/dev-docs/add-rtd-submodule.html#using-event-listeners

Some questions that could be asked about your module:

is it really necessary to wait until getBidRequestData to get your segments? Seems to me like user segments don't depend on adunit -- you should be able to retrieve them immediately on init instead of waiting for an auction
if you can rewire your module to get segment data up-front (maybe checking for updated data at each auction_init event?) then adding a new "bidRequest" listener to the infrastructure would make sense.

patmmccann · 2021-06-23T23:00:24Z

test/spec/modules/permutiveRtdProvider_spec.js

@@ -93,25 +93,6 @@ describe('permutiveRtdProvider', function () {
        })
      }
    })
-    it('sets segment targeting for TrustX', function () {


You're just deleting tests, you need to replace them with new tests

patmmccann · 2021-06-23T23:01:53Z

modules/permutiveRtdProvider.js

@@ -121,16 +125,34 @@ function getDefaultBidderFn (bidder) {
    },
    trustx: function (bid, data, acEnabled) {
      if (acEnabled && data.ac && data.ac.length) {
-        deepSetValue(bid, 'params.keywords.p_standard', data.ac)
+        setBidderRtb('trustx', data.ac)


This is the exact logic to pass data to any generic bidder, instead of whitelabeling just trustx, please make bidder name configurable so publishers can call this function for any bidder.

This would make #7074 unnecessary

bretg · 2021-06-25T20:25:56Z

Spoke with @dreischer about this - the plan is to work in the getBidderConfig solution above.

For the record the 2nd option doesn't work because this value is global, not per-adunit.

dreischer · 2021-07-15T14:09:09Z

Hey @bretg - the latest commit includes the changes we discussed: We now set ortb2.user.data for all the bidders defined by the publisher. We use the onAuctionInitEvent hook for this. We also kept the getBidRequestData hook for those bidders where we still need to modify the bid object to set custom parameters because they don't yet support the other route (we do however still set ortb2.user.data for them in parallel). If you're happy with this I'll also create the PR for the dev docs page. Thanks!

lgtm-com · 2021-07-15T14:17:47Z

This pull request introduces 1 alert when merging 1b1e0e8 into ad36dcd - view on LGTM.com

new alerts:

1 for Superfluous trailing arguments

bretg

thanks @dreischer - it would be help to note in the docs what data is set -- this module will be a template for others.

dreischer · 2021-07-20T10:25:41Z

Thanks for reviewing @bretg - I now also updated the readme as of your suggestion

* Update Premutive <> TrustX integration * remove old TrustX code * use ortb2 for trustx * enable ortb2 setBidderConfig * remove superfluous argument * Readme update

dreischer added 2 commits June 10, 2021 12:28

Update Premutive <> TrustX integration

80cf061

remove old TrustX code

5b9f3a2

patmmccann requested a review from bretg June 10, 2021 11:40

patmmccann added maintenance taxonomy labels Jun 10, 2021

robertrmartinez assigned Fawke Jun 10, 2021

robertrmartinez requested a review from Fawke June 10, 2021 15:11

Fawke approved these changes Jun 22, 2021

View reviewed changes

Fawke assigned bretg Jun 22, 2021

Merge branch 'master' of github.com:permutive/Prebid.js into permutiv…

1dea8ee

…e-trustx-update

use ortb2 for trustx

744f378

bretg mentioned this pull request Jun 22, 2021

Enhance RTD infrastructure to support bidRequest event #7078

Closed

patmmccann mentioned this pull request Jun 23, 2021

Pubmatic Bid Adapter: support permutive RTD #7074

Closed

1 task

patmmccann requested changes Jun 23, 2021

View reviewed changes

patmmccann mentioned this pull request Jun 26, 2021

AirGrid RTD Submodule: Initial Release #7108

Merged

1 task

enable ortb2 setBidderConfig

1b1e0e8

remove superfluous argument

77948d4

bretg approved these changes Jul 19, 2021

View reviewed changes

Readme update

341e7d2

patmmccann approved these changes Jul 20, 2021

View reviewed changes

dreischer mentioned this pull request Jul 28, 2021

Permutive RTD module docs prebid/prebid.github.io#3155

Merged

Fawke merged commit 3bbcf66 into prebid:master Jul 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permutive RTD submodule: TrustX update #7001

Permutive RTD submodule: TrustX update #7001

dreischer commented Jun 10, 2021

patmmccann commented Jun 10, 2021 •

edited

Loading

dreischer commented Jun 10, 2021

patmmccann commented Jun 10, 2021

dreischer commented Jun 11, 2021

Fawke commented Jun 15, 2021 •

edited

Loading

bretg commented Jun 17, 2021

PWyrembak commented Jun 18, 2021

Fawke left a comment

patmmccann commented Jun 22, 2021 •

edited

Loading

dreischer commented Jun 22, 2021

dreischer commented Jun 22, 2021

patmmccann commented Jun 22, 2021 via email

bretg commented Jun 22, 2021

patmmccann Jun 23, 2021 •

edited

Loading

patmmccann Jun 23, 2021 •

edited

Loading

bretg commented Jun 25, 2021

dreischer commented Jul 15, 2021

lgtm-com bot commented Jul 15, 2021

bretg left a comment

dreischer commented Jul 20, 2021

Permutive RTD submodule: TrustX update #7001

Permutive RTD submodule: TrustX update #7001

Conversation

dreischer commented Jun 10, 2021

Type of change

patmmccann commented Jun 10, 2021 • edited Loading

dreischer commented Jun 10, 2021

patmmccann commented Jun 10, 2021

dreischer commented Jun 11, 2021

Fawke commented Jun 15, 2021 • edited Loading

bretg commented Jun 17, 2021

PWyrembak commented Jun 18, 2021

Fawke left a comment

Choose a reason for hiding this comment

patmmccann commented Jun 22, 2021 • edited Loading

dreischer commented Jun 22, 2021

dreischer commented Jun 22, 2021

patmmccann commented Jun 22, 2021 via email

bretg commented Jun 22, 2021

patmmccann Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

patmmccann Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

bretg commented Jun 25, 2021

dreischer commented Jul 15, 2021

lgtm-com bot commented Jul 15, 2021

bretg left a comment

Choose a reason for hiding this comment

dreischer commented Jul 20, 2021

patmmccann commented Jun 10, 2021 •

edited

Loading

Fawke commented Jun 15, 2021 •

edited

Loading

patmmccann commented Jun 22, 2021 •

edited

Loading

patmmccann Jun 23, 2021 •

edited

Loading

patmmccann Jun 23, 2021 •

edited

Loading