Data Controller Module: initial release #8484

SKOCHERI · 2022-05-26T20:47:43Z

Type of change

Bugfix
Feature
New bidder adapter
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Does this change affect user-facing APIs or examples documented on http://prebid.org?
Other

Description of change

test parameters for validating bids

{
  bidder: '<bidder name>',
  params: {
    // ...
  }
}

Be sure to test the integration with your adserver using the Hello World sample page.

contact email of the adapter’s maintainer
official adapter submission

For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide:

A link to a PR on the docs repo at https://github.com/prebid/prebid.github.io/

Other information

patmmccann · 2022-05-26T20:53:37Z

modules/dataControllerModule/index.md

+pbjs.setConfig({
+    dataController: {
+       filterEIDwhenSDA: ['*']
+       filterSADwhenEID: ['id5-sync.com'] 


patmmccann · 2022-05-26T20:54:01Z

modules/dataControllerModule/index.md

+|  param name | type  | Scope | Description | Params |
+| :------------ | :------------ | :------ | :------ | :------ |
+|  filterEIDwhenSDA  | function | optional | Filters user EIDs based on SDA | bidrequest |
+|  filterSADwhenEID  | function | optional | Filters SDA based on configured EIDs | bidrequest  |


patmmccann · 2022-05-26T20:54:33Z

modules/dataControllerModule/index.js

+    if(filterEIDs) {
+      config.setConfig({'dcUsersAsEids': filterEIDs});
+    }
+  } else if (dataControllerConfig.filterSADwhenEID) {


@patmmccann do we want to have the param as "supressSDAwhenEID"/"supressEIDwhenSDA"? Filter would be more generic rather than suppress.

All references to SAD should be changed to SDA

patmmccann · 2022-05-26T20:56:51Z

Thanks so much!

patmmccann · 2022-05-26T21:57:41Z

SAD is a typo for SDA

…

On Thu, May 26, 2022, 5:16 PM SKOCHERI ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In modules/dataControllerModule/index.js <#8484 (comment)>: > + + if (!dataControllerConfig) { + processBidderRequests.getHooks({hook: processBidderRequestHook}).remove(); + return; + } + + if (dataControllerConfig.filterEIDwhenSDA && dataControllerConfig.filterSADwhenEID) { + processBidderRequests.getHooks({hook: processBidderRequestHook}).remove(); + return; + } + if (dataControllerConfig.filterEIDwhenSDA) { + let filterEIDs = submodule.filterEIDs(bidRequest) + if(filterEIDs) { + config.setConfig({'dcUsersAsEids': filterEIDs}); + } + } else if (dataControllerConfig.filterSADwhenEID) { @patmmccann <https://github.com/patmmccann> do we want to have the param as "supressSDAwhenEID"/"supressEIDwhenSDA"? Filter would be more generic rather than suppress. — Reply to this email directly, view it on GitHub <#8484 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAM25Z4JHYEGUA743Q4C45TVL7S3PANCNFSM5XCKAN3A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

dgirardi · 2022-06-15T14:54:20Z

modules/dataControllerModule/index.js

+import {processBidderRequests} from '../../src/adapters/bidderFactory.js';
+import {logError} from '../../src/utils.js';
+
+let submodules = [];


why is this planning for submodules? I'm not sure I understand the scope of the feature - going by this proposal it seems self-contained to me.

@dgirardi The proposal mentions about Publisher having ability to define the rules to filter EID and SDA hence providing submodule.

I don't understand - publishers do not have access to the module system and they wouldn't be able to add a submodule (they would have to fork the repo).

I would expect them to define the rules with setConfig, and I don't see the need multiple submodules to read that config unless it makes sense to want only part of that proposal - but I don't see anything in it that suggests that.

Will take a look

dgirardi · 2022-06-15T15:04:38Z

modules/dataControllerModule/index.js

+  dataControllerConfig = config.getConfig(MODULE_NAME);
+
+  if (!dataControllerConfig) {
+    processBidderRequests.getHooks({hook: processBidderRequestHook}).remove();


I'd prefer to keep configuration and business logic separate - this will get confusing fast as complexity increases. (IMO it makes more sense to turn the module off from the same place you turn it on).

dgirardi · 2022-06-15T15:09:18Z

src/adapters/bidderFactory.js

@@ -296,6 +296,12 @@ export function newBidder(spec) {
 * @param onCompletion {function()} invoked once when all bid requests have been processed
 */
 export const processBidderRequests = hook('sync', function (spec, bids, bidderRequest, ajax, wrapCallback, {onRequest, onResponse, onError, onBid, onCompletion}) {
+  let updatedUserAsEids = config.getConfig('dcUsersAsEids');


This looks like it's using config as a global variable, plus it's adding dead code for those who are not using the data controller module.

Why not do this directly from the hook added by the module?
Also, IMO the makeBidRequests hook is a better place for this - I don't know if it makes a difference now, but my instinct is that the earlier they have this data the better.

@dgirardi I have update this module to be self contained.
The SDA were not available if this module was called with makeBidRequests as the *RTBProviders are invoked until then

dgirardi · 2022-06-15T15:12:30Z

test/spec/modules/dataController_spec.js

+let dataControllerConfig;
+const ALL = '*';
+
+function filterEIDs(requestObject) {


This is still a work in progress I assume - this logic looks like it belongs in the module.

This module development is completed. The test contains how the submodule looks like

the test is testing test code? that can't be right. I'm missing the point entirely, once the bundle is built this code (and therefore the tests on it) are completely irrelevant no? the module in the actual codebase is an empty shell.

dgirardi

Thanks for the update. I think the approach would not work correctly (sorry if I missed it in the previous review) - for two reasons:

processBidderRequest does not run for Prebid Server bids - only client adapters;
since prebid 7, setBidderConfig (or setConfig) is not the right way to update first party data - it will not take effect until the next auction (see this)

I think the best way to do this is to hook on startAuction and:

perform filterSDA by modifying the ortb2Fragments parameter;
perform filterEIDs by modifying the adUnits[].bids[].userIdAsEid parameter - in the same way that property is set by the userId module

dgirardi · 2022-06-28T14:53:08Z

modules/dataControllerModule/index.js

+const MODULE_NAME = 'dataController';
+let dataControllerConfig;
+
+const _logInfo = createLogInfo(LOG_PRE_FIX);


there is a prefixLog utility function that does something similar already.

@dgirardi Have updated the module to be called before startAuction and fixed other issues pointed out.

dgirardi · 2022-06-28T15:00:05Z

modules/dataControllerModule/index.js

+  }
+}
+
+export function initHook() {


I think the better pattern is config.getConfig(MODULE_NAME, (cfg) => {if (isValid(cfg) enable() else disable())}) - it's the approach taken by most (if not all) other modules:

it keeps better separation between setup logic and business logic

it doesn't use cpu cycles during the auction in the case when the module has not been enabled

it needs to worry about fewer edge cases, like what happens if configuration is set during (or after) the auction

dgirardi · 2022-06-28T15:02:21Z

modules/dataControllerModule/index.js

+}
+
+function hasValidConfiguration() {
+  dataControllerConfig = config.getConfig(MODULE_NAME);


may I suggest something like setupConfig instead of hasValidConfiguration? to make it clear that the function has side effects.

dgirardi · 2022-07-07T17:23:18Z

modules/dataControllerModule/index.js

+
+function filterSDA(adUnits, ortb2Fragments) {
+  let bidderEIDSMap = getEIDsSource(adUnits);
+  for (const [key, value] of Object.entries(ortb2Fragments.bidder)) {


should this also remove user.data from global configuration, if it's not set by bidder? I'm not sure what the intent of the feature is.

If as the pub I do

setConfig({ ortb2: { user: { data: [... stuff here ..] } } })

and assuming there's nothing else, here ortb2Fragments would come in as:

{ global: {user: {data: [.. stuff ..]}}, bidder: {} }

so this logic would have no effect, and downstream in the adapters the final ortb2 object would still contain user.data. Is that correct?

value.user.data = [] should be deepSetValue(value, 'user.data', []), otherwise it can throw if any of the intermediates is missing. (Or should it be delete value?.user?.data - should it be empty, or missing?)

dgirardi · 2022-07-07T17:33:18Z

modules/dataControllerModule/index.js

+export function getSegmentConfig(ortb2Fragments) {
+  let bidderSDAMap = new Map();
+
+  for (const [key, value] of Object.entries(ortb2Fragments.bidder)) {


Same here - should this look at global config?

dgirardi · 2022-07-07T18:43:11Z

Reading through https://docs.google.com/document/d/1g9GxuMtPwZyk5lRKCyoWVaYJGpqUGEbFNwEBMQswcjU/edit# again, I think the intent is - from the point of view of the publisher, "I don't want this data to be visible to the adapters / SSP".

So I think this module should be overzealous, and:

inspect / remove global configuration as well (ortb2Fragments.global)
also remove data from user.ext.eids and bid.userId, not just bid.userIdAsEid. otherwise, there might be a situation where for example an RTD module is populating the first, or an adapter is looking at the second (instead of userIdAsEid), and they still get access to the "forbidden" data.

SKOCHERI · 2022-07-07T19:10:46Z

Reading through https://docs.google.com/document/d/1g9GxuMtPwZyk5lRKCyoWVaYJGpqUGEbFNwEBMQswcjU/edit# again, I think the intent is - from the point of view of the publisher, "I don't want this data to be visible to the adapters / SSP".

So I think this module should be overzealous, and:

inspect / remove global configuration as well (ortb2Fragments.global)

also remove data from user.ext.eids and bid.userId, not just bid.userIdAsEid. otherwise, there might be a situation where for example an RTD module is populating the first, or an adapter is looking at the second (instead of userIdAsEid), and they still get access to the "forbidden" data.

Updating to consider the above

SKOCHERI · 2022-07-08T18:03:14Z

user.ext.eids

@dgirardi I am trying to create request with user.ext.eids to test. Is this the right path where user eids are set "ortb2Fragments.bidder..user.ext.eids"

dgirardi · 2022-07-08T18:16:28Z

they could be in either bidder or global, and both should be checked / filtered, like the other pieces of data. The logic is roughly speaking, when the auction starts, global is from getConfig, bidder is from getBidderConfig; then the object passes through FPD and RTD modules, then it gets here in dataController (which should be last). Any of the above may have added anything in them, so this should check global.user and each entry in bidder.[code].user. At the end each adapter sees only one object ortb2 = mergeDeep({}, ortb2Fragments.global, ortb2Fragments.bidder.[code]).

SKOCHERI · 2022-07-08T21:49:34Z

they could be in either bidder or global, and both should be checked / filtered, like the other pieces of data. The logic is roughly speaking, when the auction starts, global is from getConfig, bidder is from getBidderConfig; then the object passes through FPD and RTD modules, then it gets here in dataController (which should be last). Any of the above may have added anything in them, so this should check global.user and each entry in bidder.[code].user. At the end each adapter sees only one object ortb2 = mergeDeep({}, ortb2Fragments.global, ortb2Fragments.bidder.[code]).

@dgirardi Updated to consider global and bidder segments and update eids as mentioned above

dgirardi

LGTM - I think this conforms to the proposal now as I understand it. I have a few more minor suggestions, I think the first (changing from GLOBAL = 'global' to GLOBAL = {}) should be done for correctness, but the rest is not that important.

dgirardi · 2022-07-11T16:48:51Z

modules/dataControllerModule/index.js

+const LOG_PRE_FIX = 'Data_Controller : ';
+const ALL = '*';
+const MODULE_NAME = 'dataController';
+const GLOBAL = 'global';


You can use an empty object as a unique Map key (const GLOBAL = {}), it has the advantage that it cannot clash with, for example, a custom alias bidder named 'global'.

dgirardi · 2022-07-11T16:52:32Z

modules/dataControllerModule/index.js

+    return false;
+  }
+  let containsEIDs = false;
+  _dataControllerConfig.filterSDAwhenEID.forEach(source => {


This could be rewritten as return _dataControllerConfig.filterSDAwhenEID.some((source) => bidderEids.has(source)).

.forEach always loops over the whole iterable; .some will stop as soon as the predicate returns true.

dgirardi · 2022-07-11T16:54:11Z

modules/dataControllerModule/index.js

+  if (bidderSegement == undefined) {
+    return false;
+  }
+  _dataControllerConfig.filterEIDwhenSDA.forEach(segment => {


same, .some would be more succinct and more efficient here.

dgirardi · 2022-07-11T17:02:59Z

modules/dataControllerModule/index.js

+}
+
+function getSegmentConfig(ortb2Fragments) {
+  let bidderSDAMap = new Map();


there's some duplication in here, which you could avoid by reusing the same logic for both global and bidder data. Here's a possible refactor to illustrate what I mean:

let bidderSDAMap = new Map(); function collectSegments(key, data) { let segmentSet = constructSegment(deepAccess(data, 'user.data') || []); if (segmentSet && segmentSet.size > 0) bidderSDAMap.set(key, segmentSet); } collectSegments(GLOBAL, ortb2Fragments.global); Object.entries(ortb2Fragments.bidder).forEach(([bidder, data]) => collectSegments(bidder, data)); return bidderSDAMap;

mmoschovas · 2022-07-14T16:33:23Z

modules/dataControllerModule/index.md

+# Description
+
+This module will filter EIDs and SDA based on the configurations.
+The filtered EIDs are stored in 'dcUsersAsEids' configuration and filtered SDA are updated in bidder configuration.


Is this detail no longer relevant? I am not seeing any of this stored any longer, so if that is the case, can this be removed

mmoschovas · 2022-07-14T16:38:46Z

modules/dataControllerModule/index.js

+      return;
+    }
+    confListener(); // unsubscribe config listener
+    _dataControllerConfig = dataControllerConfig.dataController;


I think we can clean this up a little to store the dataController in a const then reference that instead or referencing the full path each time.

i.e.

const dataController = dataControllerConfig && dataControllerConfig.dataController; if (!dataController) { ... } if (dataController.filterEIDwhenSDA && dataController. filterSDAwhenEID) { ... } confListener(); // unsubscribe config listener _dataControllerConfig = dataController; getHook('startAuction').before(filterBidData);

mmoschovas

Thanks for the update! lgtm

* Data Controller Module * Data Controller Module * Correcting typo * Setting hook based on data controller configuration * Setting hook based on data controller configuration * Updating to filter data before startAuction event * Updating to filter data before startAuction event * Review comment fixes * Review comment fixes Co-authored-by: skocheri <skocheri@rubiconproject.com>

mikemangas · 2022-08-08T12:58:16Z

@SKOCHERI
@dgirardi
@mmoschovas

Thanks for your work. I would like to ask you some questions.

I am not sure, how can i implement and test this module. What are the Requirements for the Data Controller Module?
Are Seller Defined Audiences (SDA) required, in order to test/use this module? If yes, Is there a way to add SDA (provider name, segment id, taxonomy name) to my bid requests? if yes, is there a documentation for that?
What do i pass into "filterEIDwhenSDA" or "filterSDAwhenEID"? What are the requirements for using the one or the other?

Thank you for any helpful information.

MartianTribe · 2022-08-10T15:44:02Z

Docs PR:

prebid/prebid.github.io#3928

mikemangas · 2022-09-07T09:04:00Z

Does a bidAdapter have to change the code, in order to support this module?

* Data Controller Module * Data Controller Module * Correcting typo * Setting hook based on data controller configuration * Setting hook based on data controller configuration * Updating to filter data before startAuction event * Updating to filter data before startAuction event * Review comment fixes * Review comment fixes Co-authored-by: skocheri <skocheri@rubiconproject.com>

Data Controller Module

fcf32fb

patmmccann reviewed May 26, 2022

View reviewed changes

patmmccann changed the title ~~Data Controller Module~~ Data Controller Module: initial release May 26, 2022

patmmccann added feature needs docs needs 2nd review Core module updates require two approvals from the core team labels May 26, 2022

Data Controller Module

4a08291

skocheri added 2 commits June 1, 2022 10:08

Correcting typo

5aecd26

Setting hook based on data controller configuration

5332b4c

ChrisHuie requested a review from dgirardi June 8, 2022 14:46

ChrisHuie assigned dgirardi Jun 8, 2022

dgirardi reviewed Jun 15, 2022

View reviewed changes

Setting hook based on data controller configuration

9f7622b

dgirardi requested changes Jun 28, 2022

View reviewed changes

Merge branch 'master' into DC

217c366

ChrisHuie self-requested a review July 6, 2022 11:24

ChrisHuie self-assigned this Jul 6, 2022

Updating to filter data before startAuction event

1d49b72

dgirardi reviewed Jul 7, 2022

View reviewed changes

patmmccann linked an issue Jul 8, 2022 that may be closed by this pull request

Conditional permissioning of id's & data #6240

Closed

Updating to filter data before startAuction event

b4f7174

dgirardi approved these changes Jul 11, 2022

View reviewed changes

Review comment fixes

798b918

ChrisHuie removed their request for review July 13, 2022 13:21

ChrisHuie assigned mmoschovas and unassigned ChrisHuie Jul 13, 2022

ChrisHuie requested review from mmoschovas and Fawke July 13, 2022 13:21

ChrisHuie assigned Fawke Jul 13, 2022

mmoschovas reviewed Jul 14, 2022

View reviewed changes

Review comment fixes

f5b14be

mmoschovas approved these changes Jul 15, 2022

View reviewed changes

patmmccann merged commit 715a1f5 into prebid:master Jul 25, 2022

mikemangas mentioned this pull request Sep 7, 2022

Data Controller Module Questions #8959

Closed

Data Controller Module: initial release #8484

Data Controller Module: initial release #8484

Conversation

SKOCHERI commented May 26, 2022

Type of change

Description of change

Other information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patmmccann commented May 26, 2022

patmmccann commented May 26, 2022 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgirardi Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SKOCHERI Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgirardi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgirardi Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgirardi commented Jul 7, 2022

SKOCHERI commented Jul 7, 2022

SKOCHERI commented Jul 8, 2022

dgirardi commented Jul 8, 2022

SKOCHERI commented Jul 8, 2022

dgirardi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmoschovas Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmoschovas left a comment

Choose a reason for hiding this comment

mikemangas commented Aug 8, 2022

MartianTribe commented Aug 10, 2022

mikemangas commented Sep 7, 2022

dgirardi Jun 15, 2022 •

edited

Loading

SKOCHERI Jun 24, 2022 •

edited

Loading

dgirardi Jul 7, 2022 •

edited

Loading

mmoschovas Jul 14, 2022 •

edited

Loading