Prebid Core: switch native assets to ortb2 format #8086

musikele · 2022-02-18T11:37:07Z

NOTE: this is a continuation of PR #7847. Original submitter ceased working on adtech and I am continuing his work. In order to handle change requests and future work on this, I decided (with the consent of @bretg) to move in a new PR. Original thread of comments and reviews is in the old one.

Type of change

Feature

Description of change

Change from prebid 'standard' of defining native assets to openRTB 1.2 standard.
Instead of sponsoredBy, image, icon, we would have openRTB equivalents:

{ data: { type: 1 }}
{ img: { type: 3 }}
{ img: { type: 1 }}

This will help standardize prebid native. Also, a lot of bidders are sending ortb2 requests on their back end, so at the moment, they are converting bid request from prebid to openRTB standard and later they are converting bid responses from openRTB to prebid standard. With this approach a lot of duplicated code between the bidders can be removed.

Backwards compatibility

When this PR is merged, we will still have backwards compatibility. Which mean, nothing will break for the existing native bidders. That will give us some time until all bidders adjust their code

Other information

This PR resolves #7830 issue.

musikele · 2022-02-18T11:40:08Z

My latest commits are in response of this comment by @bretg on the old PR #7847:

Ok, discussed in today's PBJS meeting. We propose handling this native change in a way similar to how we handled the first party data change last year:

Add some transition code (e.g. toOrtbNative) that maps old-style AdUnit native definitions to new-style.
Create a transition function (e.g. fromOrtbNative) that bid adapters can use to translate the new-style native definition back to the old-style. This is a lossy transition and would only support whatever fields are currently covered by the proprietary format.
Update all 74 native-capable bid adapters to call fromOrtbNative() at the top of buildRequests. Then we communicate with them to start the process of converting their adapters to not need fromOrtbNative().

FWIW, I poked around a couple of adapters: one had a complicated bunch of code for mapping native params and the other seemed to wrap it all up and parse server-side. So this transition may take a while.

Documentation-wise, the proposal is:

produce a new document that is the recommended way that new native implementations should be
leave the current doc in place but warn that new implementations should be done in the ortb-way
hide the old-old doc away, making it more clear that no one should be using that (sendId) approach anymore.

lgtm-com · 2022-02-18T12:06:55Z

This pull request introduces 1 alert when merging 7e8a6c5 into 5eacb78 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

musikele · 2022-02-18T12:48:11Z

Docs pr: prebid/prebid.github.io#3497

musikele · 2022-02-18T12:49:16Z

Prebid Universal Creative PR: prebid/prebid-universal-creative#150

dgirardi · 2022-03-01T16:51:28Z

test/spec/unit/pbjs_api_spec.js

-          body: {required: false},
-          icon: {required: false},
-        });
+        expect(nativeRequest.ortb.assets).to.deep.equal([


Assuming ortb and "legacy" definitions are mutually exclusive, should we add some validation for that? The best place is probably checkAdUnitSetup (in prebid.js and also the sizeMappingV2 module). Or what is the expected behavior if I mix them?

I don't think they should be mutually exclusive. Thousands of publishers already have native ad units set up in the old way, but in the future bidders will start to switch to ortb2, so for some time the same ad unit will contain both types.

Then, it's the bidder's responsibility to decide which version they want to use. I think that ortb will be the preferred format since it's more expressive, but waiting for bidders to migrate can take ages. That's why we've also given the two functions to convert to and from legacy prebid native format.

@bretg wdyt ?

But a publisher is expected to specify either one or the other for a given adunit, right? otherwise it's not clear how you'd "merge" the definitions, or what to do in case they conflict. That's what I meant by validation.

I think publishers may specify both types in the same ad unit. This shouldn't be seen as an error. It's the single adapter that will have to adapt its code to both formats.

Right now publishers are using only the proprietary format. When this PR is merged, the vast majority of these publishers will continue to use the proprietary format. Then, a bunch of native bid adapters will start to support the ortb format and will ask publishers to bid in this new format. So we'll have the situation where, for the same ad unit, some native bidders will understand the old way only, others will understand both ways, and some others will understand the new way only. It's up to them to decide how to interpret the assets object.

If I were a bidder, i'd implement this logic:

let ortb; if (nativeType.native && nativeType.native.ortb) { // use only ortb {ortb} = nativeType.native; } else { // publisher is using old proprietary format, it's up to them to // decide wether use it straight or convert to ortb with function // `toOrtbNative()` ortb = toOrtbNative(nativeType.native) }

Wouldn't it be better for a publisher to switch over to ortb, and let the translation shim you already added in here handle the adapters that have not yet been ported? With the goal of having every bid adapter talk only ortb at some point in the future. Having to maintain two versions would add a lot of friction and I don't see the need for it.

To clarify, I see the transition as:

adapters initially understand the old way only (and this PR makes sure they get it regardless of how the adunit is defined)

gradually, adapters will understand the new way only (and they can translate old definitions as you show above)

But I don't think publishers or adapters should be interested in mixing or matching the formats; it's going to invite inconsistencies otherwise.

I agree with @dgirardi in the respect that a publisher should define each PBJS AdUnit with either the old or the new methods, but not a mix.

However, a publisher may change have a slow and extended transition where some pages/adunits are converted to ortb while others say on the legacy way. i.e. some AdUnits may define native assets with ortb, and others may define native assets in the old way. But a given AdUnit cannot mix them.

A validation that logs a warning when an AdUnit mixes legacy and ortb definitions is possible, but I view it as somewhat expensive. The code would have to look specifically for each of the 18 asset types and warn when one of them is defined at the same time as ortb. If this doesn't add much to the bytecount (i.e. we already have an array of assetcodes to loop through), then fine, add a validation.

When it comes to the bid adapters, I think we're all on the same page: bidders should work towards support ortb2 and having the utility functions will enable them to handle the transition period where they might see a mix of old and new.

I think validation might be as simple as "if it contains ortb, it should not contain any other key" - but if it's not, point taken that it may not be worth the bytes.

@dgirardi added the code to check if ortb is in use, no legacy native keys should be used; this has been done here and it's called by checkAdUnitSetup in both prebid and sizeMappingV2

dgirardi · 2022-03-01T16:52:13Z

test/spec/unit/secureCreatives_spec.js

@@ -472,6 +476,7 @@ describe('secureCreatives', () => {
        describe(`for ${test} bids`, () => {
          beforeEach(() => {
            prepBid(adResponse);
+            // throw JSON.stringify(adResponse);


This looks unintentional.

dgirardi · 2022-03-01T16:56:37Z

test/spec/unit/secureCreatives_spec.js

@@ -332,8 +332,8 @@ describe('secureCreatives', () => {
        sinon.assert.calledWith(stubGetAllAssetsMessage, data, adResponse);
        sinon.assert.calledOnce(ev.source.postMessage);
        sinon.assert.notCalled(stubFireNativeTrackers);
-        sinon.assert.neverCalledWith(stubEmit, CONSTANTS.EVENTS.BID_WON);
-        sinon.assert.notCalled(spyAddWinningBid);
+        sinon.assert.calledWith(stubEmit, CONSTANTS.EVENTS.BID_WON, adResponse);


IMO it's better to remove these assertions and add new tests specific to the bid won event. Because conceptually we changed from "this event should fire it, this one also, this one not" to "any adId should fire the first time but not the second". So it's a bit confusing to see them here as the precondition is not explicit.

added new test here: https://github.com/prebid/Prebid.js/pull/8086/files#diff-216136cd3abd45d8370399ff0c8cec9a603ad18597008c38c0d54e503eafe7c3R340

dgirardi · 2022-03-01T17:04:41Z

test/spec/unit/secureCreatives_spec.js


        expect(adResponse).to.have.property('status', CONSTANTS.BID_STATUS.RENDERED);
+
+        // revert back to 'adId = 1'
+        adResponse.adId = bidId;


This should be in some setup / teardown section, or the next guy will also have to curse for however long it took you to track it down :)

dgirardi · 2022-03-01T17:22:44Z

src/native.js

+  const bidRequest = index.getAdUnit(bid);
+  if (!bidRequest) { return false; }
+
+  if (deepAccess(bid, 'native.ortb') && deepAccess(bidRequest, 'nativeParams.ortb')) {


This seems to enforce that an ortb response is valid only if the request was also in ortb. But during the transition period I imagine we'll have to deal with a lot of "legacy" requests, which would force adapters to individually have logic like "if the request is legacy, respond in legacy; otherwise respond in ortb". Is it possible to decouple them? maybe by falling back to the conversion logic if the request does not have nativeParams.ortb.

If I understand this right with this PR no adapter is hitting this code path. Could/should the PBS adapter respond in ortb?

Also a minor point, I don't like the name bidRequest for an ad unit.

if ortb keyword is not present in nativeParams, prebid will assume that the request was made using the legacy format. What do you mean by "decouple" the two request types? Do you mean to convert the request to OpenRTB and then validate the response against the just-created openRTB request?

With this PR we will adapt all adapters to translate ortb to legacy format request. PBS will understand both types, checkout prebidServerBidAdapter/index.js .

I will change bidRequest to adUnit.

1 - yes, because as adapters transition, they will ideally want to deal only with ORTB - so for a legacy request they will convert to ortb and reply with ortb. This check would now be skipped if the request is legacy.
2. But the PBS adapter is still replying with legacy responses. Should it be converted over to ortb completely? (should it even deal with legacy requests, beside converting them?)

All responses are converted to Ortb.

Prebid Server was already responding in Ortb. I only had to remove the code that converted to legacy.

dgirardi · 2022-03-01T17:38:43Z

src/native.js

  };

+  if (adObject.native.ortb) {
+    Object.keys(adObject.native).forEach(key => {
+      message[key] = adObject.native[key];


Is this right? From my reading of the PUC PR it looks like it expects these under message.ortb ?

Actually disregard this, of course ortb is also being copied over.

lgtm-com · 2022-04-26T11:43:46Z

This pull request introduces 2 alerts when merging 7bcbb52 into f395eac - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

dgirardi · 2022-05-02T14:54:18Z

src/constants.json

+    "salePrice": "saleprice",
+    "displayUrl": "displayurl"
+  },  
+  "ASSET_TYPES": {


I'd put NATIVE somewhere in this name, but maybe it's clearer than it looks to me

dgirardi · 2022-05-02T15:04:56Z

src/secureCreatives.js

@@ -108,6 +109,13 @@ function handleNativeRequest(reply, data, adObject) {
    logError(`Cannot find ad '${data.adId}' for x-origin event request`);
    return;
  }
+
+  if (!WON_AD_IDS.has(adObject.adId)) {


WON_AD_IDS could be a WeakMap using adObject as key - otherwise it's unbounded. Not a big deal as I don't think we ever throw adObject to the garbage collector, but one day we might.

With weakmaps, i'd have to change line 114 to WON_AD_IDS.set(adObject, adObject) and honestly i don't like the repetition :)

Since the complexity is still sublinear (as written in the ecmaspec), i preferred to use WeakSet, because it maps more naturally to what we're doing here. What do you think? the only major problem was compatibility with IE, but Prebid already dropped it in version 6.

dgirardi · 2022-05-02T15:12:47Z

modules/prebidServerBidAdapter/index.js

@@ -561,8 +555,8 @@ Object.assign(ORTB2.prototype, {
      this.adUnitsByImp[impressionId] = adUnit;

      const nativeParams = adUnit.nativeParams;
-      let nativeAssets;
-      if (nativeParams) {
+      let nativeAssets = nativeAssetCache[impressionId] = deepAccess(nativeParams, 'ortb.assets');


GH does not let me pick the right line for this - further down here, line 681, the code path for native packages only ortb.assets into an ORTB native request. Does that still make sense, or should it just say request.native = nativeParams.ortb? if no, why not? maybe explained in the form of a unit test? :)

You are right! Before, there was no way for publishers to pass context, plcmttype and other fields into a media type object. With my latest commit the mediaType.native.ortb will contain exactly that, so if that's present, i'm going to pass that to the request, otherwise we go the old way.

dgirardi · 2022-05-03T16:27:03Z

modules/prebidServerBidAdapter/index.js

              // TODO: determine best way to pass these and if we allow defaults
              context: 1,
              plcmttype: 1,
              eventtrackers: [
-                {event: 1, methods: [1]}
+                { event: 1, methods: [1] }


After reading about the meaning of these fields I think it's safer if we keep these as a default - eventtrackers especially, as it does not sound like something the publisher would be interested in specifying.

{event: 1, methods: [1]} should mean "Prebid supports pixel tracking for impression views". I think it should be changed to {event: 1, methods: [1, 2]} to signal support for js trackers as well; other events are related to video which the PBS adapter does not support for native.

We may be able to derive context and plcmttype from other properties of the adUnit - but I don't think that needs to be in scope for this PR, and if you keep 1/1 you know at least you won't break it more than it already is.

As discussed yesterday, I've left {event: 1, methods: [1]} because it's a more conservative choice, and also pixels are generally more accepted than the JS equivalent.

robertrmartinez · 2022-05-12T17:40:03Z

modules/appnexusBidAdapter.js

@@ -123,6 +124,9 @@ export const spec = {
   * @return ServerRequest Info describing the request to the server.
   */
  buildRequests: function (bidRequests, bidderRequest) {
+    // convert Native ORTB definition to old-style prebid native definition
+    bidRequests = convertOrtbRequestToProprietaryNative(bidRequests);


This looks like it is overwriting the incoming bidRequests. Should it be just creating a copy instead here? Not sure is this affects downstream prebid core if these input bidRequests object are changed?

For example, if the input has rendererUrl / sendTargetingKeys: false

Then this function overwrites them and they are removed, does the core still handle the bidResponse correctly when it maps back to ortb2 etc?

Looks like the function changes the incoming object anyways, so prob don't need to set it equal to it if it changes it anyways?

Maybe a copy should be returned so the original bidRequests object is not changed?

Hi @robertrmartinez
You are right, it is overwriting the incoming BidRequests. I tried to create a copy and pass it around, and this would be more correct (from a functional standpoint) and may lead to less bugs, however it is unpractical.

Some bidders in their tests rely on the fact that the bidRequest object they sent is exactly the one that will come back; some check the reference, other check some properties that they are attaching to the object; in the end, creating a copy of bidRequests instead of modifying it in place would mean to fix all their tests.

Regarding the fact that rendererUrl/sendTargetingKeys may disappear: I'll do a change in the code so this will not happen.

To summarize, my idea is that we should create a copy, but this is impossible now, because it will break all the tests that assume that the bidRequest they're testing is the same they sent in input.

@robertrmartinez check the code now!

That all makes sense to me!

Hopefully this is a short term solution and ORTB gets fully adopted and legacy native mediatype is deprecated!

@musikele would a compromise of "copy-on-write" work? since tests should not be setting up native ortb2 right now, if you return the original object when no conversion is necessary, but clone it when it is, I think it might work?

@dgirardi I've implemented copy-on-write and tests are not failing.

musikele · 2022-05-24T13:52:22Z

rebased

…templates

jsnellbaker

LGTM. Thanks for putting this together.

I think for the 'changes' that are desired for bid adapters to adopt, it would be great if there was a 'cheat-sheet' to summarize/highlight what changes are more strongly desired vs nice-to-have (and if there are dates involved around changing the baseline in a future major release).

musikele · 2022-07-26T13:12:43Z

@jsnellbaker will follow shortly

* switch native assets to ortb2 format * put rendererUrl, adTemplate back in message * rename ortb2 to ortb * typo fix * changes to support ortb for native media type * handle bid won case * mark bid as won on any post message for native * fix tests for bid_won event for native * add missing imports * native converting functions * convert new native ortb media type object to proprietary format for native bidders * fix LGTM.com issue * added comments; minor fixes * added test for convertOrtbRequestToProprietaryNative + fixes * add nativeParams to conversion * support nativeParams * check that when native.ortb is present, it's the only property * remove commented code * removed unnecessary tests * added test that checks that BID_WON is not fired more than once for the same adId * validation is now performed on ortb data only * fix for prebidServer_native_example.html * PrebidServerBidAdapter also responds in ortb format * fix aspect_ratios as an array in tests * LGTM fix - remove unused variables * Better name for native constants * use WeakSet instead of Set * when native request is openRTB, the whole native.ortb is passed over * retain some defaults dor native PBS request * fix for empty ortbRequest in nativeBidIsValid * add non-asset properties to media type object * final fixes after rebasing * handle tracking of soon-deprecated ortb imptrackers and jstracker * let native ad unit take all horizontal space * pass "mapping" from legacy native to ortb to prebid universal creative * Convert ortb assets to legacy for backward compatibility with legacy templates * add ortb conversion for more bid adapters * wrap conversion function in FEATURES.NATIVE * remove expired bids from native wrapper * instead of modifying bidresponse, use nativeReq * wrap test in FEATURES.NATIVE * fix for native mapping in prebidServerBidAdapter * use copy-on-write to convert ortb requests to legacy * fix comment on function * Update criteoBidAdapter.js Co-authored-by: Filip Stamenkovic <ficadub@gmail.com> Co-authored-by: Michele Nasti <michele@rtk.io> Co-authored-by: Patrick McCann <patmmccann@gmail.com>

musikele changed the title ~~Native assets ortb2~~ Prebid Core: switch native assets to ortb2 format (2) Feb 18, 2022

musikele mentioned this pull request Feb 18, 2022

Prebid Core: switch native assets to ortb2 format #7847

Closed

1 task

musikele changed the title ~~Prebid Core: switch native assets to ortb2 format (2)~~ Prebid Core: switch native assets to ortb2 format Feb 18, 2022

musikele added feature native needs 2nd review Core module updates require two approvals from the core team needs review labels Feb 18, 2022

musikele force-pushed the native_assets_ortb2 branch from 2adacd1 to 7e8a6c5 Compare February 18, 2022 11:58

ChrisHuie requested a review from dgirardi February 24, 2022 12:21

ChrisHuie assigned dgirardi Feb 24, 2022

ChrisHuie requested a review from osazos February 24, 2022 12:22

ChrisHuie assigned osazos Feb 24, 2022

dgirardi reviewed Mar 1, 2022

View reviewed changes

patmmccann added the CORE HIGH IMPACT label Mar 25, 2022

musikele force-pushed the native_assets_ortb2 branch from b98acc2 to 7bcbb52 Compare April 26, 2022 11:35

musikele requested review from bretg and dgirardi April 26, 2022 11:41

dgirardi reviewed May 2, 2022

View reviewed changes

dgirardi reviewed May 3, 2022

View reviewed changes

robertrmartinez reviewed May 12, 2022

View reviewed changes

musikele force-pushed the native_assets_ortb2 branch from be26aa1 to 80d7fca Compare May 24, 2022 13:41

musikele force-pushed the native_assets_ortb2 branch from 80d7fca to bfdbf60 Compare May 25, 2022 08:47

Michele Nasti and others added 16 commits July 25, 2022 17:47

retain some defaults dor native PBS request

5957c98

fix for empty ortbRequest in nativeBidIsValid

44cc459

add non-asset properties to media type object

5c227f9

final fixes after rebasing

55b0ae0

handle tracking of soon-deprecated ortb imptrackers and jstracker

0903aed

let native ad unit take all horizontal space

9ee2ec8

pass "mapping" from legacy native to ortb to prebid universal creative

69d80df

Convert ortb assets to legacy for backward compatibility with legacy …

366e572

…templates

add ortb conversion for more bid adapters

e273140

wrap conversion function in FEATURES.NATIVE

6991d2b

remove expired bids from native wrapper

39ed9d3

instead of modifying bidresponse, use nativeReq

ef807e7

wrap test in FEATURES.NATIVE

6ef7e1d

fix for native mapping in prebidServerBidAdapter

ca8747f

use copy-on-write to convert ortb requests to legacy

3479bf1

fix comment on function

1155c10

musikele force-pushed the native_assets_ortb2 branch from e408aa1 to 1155c10 Compare July 25, 2022 15:47

jsnellbaker approved these changes Jul 25, 2022

View reviewed changes

patmmccann added 2 commits July 25, 2022 16:23

Merge branch 'master' into native_assets_ortb2

ce6b8f3

Update criteoBidAdapter.js

74338ad

patmmccann merged commit d06bdad into master Jul 26, 2022

patmmccann deleted the native_assets_ortb2 branch July 26, 2022 13:15

dgirardi mentioned this pull request Jul 27, 2022

Prebid core: fix native trackers for Prebid Server; simplify native ORTB logic #8748

Merged

1 task

dgirardi mentioned this pull request Aug 4, 2022

Prebid core: fix bug with some native assets being lost from ortb native responses #8785

Merged

1 task

jsnellbaker mentioned this pull request Aug 23, 2022

Appnexus Bid Adapter: update logic of native viewability script #8890

Merged

1 task

dgirardi mentioned this pull request Sep 9, 2022

Prebid core: fix bug with native adUnits generating invalid PBS requests #8977

Merged

1 task

dgirardi mentioned this pull request Nov 15, 2022

Native targeting keys are not set for bids that use ORTB native responses #9246

Closed

Prebid Core: switch native assets to ortb2 format #8086

Prebid Core: switch native assets to ortb2 format #8086

Conversation

musikele commented Feb 18, 2022

Type of change

Description of change

Backwards compatibility

Other information

musikele commented Feb 18, 2022 • edited Loading

lgtm-com bot commented Feb 18, 2022

musikele commented Feb 18, 2022

musikele commented Feb 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgirardi Mar 2, 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

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

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

lgtm-com bot commented Apr 26, 2022

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 May 3, 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

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

musikele commented May 24, 2022

jsnellbaker left a comment

Choose a reason for hiding this comment

musikele commented Jul 26, 2022

musikele commented Feb 18, 2022 •

edited

Loading

dgirardi Mar 2, 2022 •

edited

Loading

dgirardi May 3, 2022 •

edited

Loading