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

Prebid server support for OpenRTB Native bids #3145

Merged
merged 11 commits into from
Jul 30, 2019

Conversation

snapwich
Copy link
Collaborator

@snapwich snapwich commented Oct 1, 2018

Type of change

  • Feature

Description of change

This is a pull-request for following the development of native support for the prebid server adapter's OpenRTB endpoint and getting feedback on how to integrate properly with Prebid.js adUnits.

Other information

OpenRTB Spec v2.4: https://www.iab.com/wp-content/uploads/2016/01/OpenRTB-API-Specification-Version-2-4-DRAFT.pdf

OpenRTB Native Spec 1.2: https://www.iab.com/wp-content/uploads/2017/04/OpenRTB-Native-Ads-Specification-Draft_1.2_2017-04.pdf

@stale
Copy link

stale bot commented Oct 16, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Oct 16, 2018
@snapwich
Copy link
Collaborator Author

Still awaiting feedback

@stale stale bot removed the stale label Oct 16, 2018
@snapwich snapwich added the pinned won't be closed by stalebot label Oct 26, 2018
@snapwich snapwich self-assigned this Jan 7, 2019
@clarkish
Copy link

clarkish commented Jun 6, 2019

hi @snapwich are there any updates on this one?

is there an ETA for the availability of OpenRTB Native via Prebid Server?

@snapwich snapwich force-pushed the prebidServer-openrtb-native branch from aa26292 to e413c45 Compare June 26, 2019 21:10
@snapwich snapwich removed the on hold label Jun 26, 2019
@snapwich
Copy link
Collaborator Author

Can't get a valid native response back from appnexus prebid server but I have unit tests around it. Need to decide how context, plcmttype, and privacy get sent for a native bid. Currently just default the first two to 1 and not setting privacy. These don't need to be resolved before merge though.

@idettman
Copy link
Contributor

Regarding: #3145 (comment)
Is this PR ready for review now?

@snapwich
Copy link
Collaborator Author

snapwich commented Jul 1, 2019

Yes, ready for review. It's possible it might need updates but is safe for merge.

@snapwich snapwich force-pushed the prebidServer-openrtb-native branch from e413c45 to 6c02b22 Compare July 12, 2019 16:54
@snapwich snapwich requested review from idettman and removed request for idettman July 12, 2019 17:11
@idettman idettman added the needs 2nd review Core module updates require two approvals from the core team label Jul 15, 2019
…b-native

# Conflicts:
#	modules/rubiconAnalyticsAdapter.js
Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

@snapwich Are there any special test params that should be used to test making a native request to PBS and seeing the response? Or should it work with any PBS-compliant adapter assuming they make a typical native adUnit?

I was trying to setup a test and while the PBS request went out, PBS did not return a bid back. The same native adUnit works when I run it through the correspondning client-side adapter.

@snapwich
Copy link
Collaborator Author

@jsnellbaker as far as I am aware prebid server might not even have full support for native bids from pbjs right now. For testing I was able to verify that native impressions were being received in prebid server but I had to mock the responses from prebid server to verify this code. I was never able to get an end-to-end fully working.

Do we have an example of native working with prebid-server and a test endpoint? If I recall correctly when coding this the appnexus endpoint would only return nobids through prebid server for native requests.

I was hoping to get this merged sooner than later (since it keeps getting merge conflicts) and work out the kinks on prebid-server as needed, but if it's possible to get an end-to-end test working I think that'd be great.

@jsnellbaker
Copy link
Collaborator

@snapwich I'm checking internally about the appnexus adapter and some native test params that work through PBS. Will follow-up on this point.

@jsnellbaker
Copy link
Collaborator

jsnellbaker commented Jul 19, 2019

@snapwich

I received some feedback; the issue that we're seeing is the absence of an eventtrackers config object in the native request payload.

Specifically, if its v1.2 and an image asset is being requested, we require eventtrackers to be in the request that gives us “permission” to track the image assets.
It should look like this:

{
  ...
  "eventtrackers": [
    { "event": 1, "methods": [1] }
  ]
}

@snapwich
Copy link
Collaborator Author

@jsnellbaker that makes sense. any feedback on a test endpoint to appnexus or test parameters I could use to verify through prebid-server -> appnexus adapter?

@jsnellbaker
Copy link
Collaborator

This is a test page I setup and was trying to use. The placements/params should return a valid appnexus native bid (they do when I run it through the client-side adapter).

demo_native.html.zip

@mkendall07
Copy link
Member

This pull request introduces 2 alerts when merging 61589da into c2bdb94 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

This project has automated code review enabled, but doesn't use the LGTM GitHub App. Migrate over by installing the app. Read about the benefits of migrating to GitHub Apps in the blog.


Comment posted by LGTM.com

@jsnellbaker
Copy link
Collaborator

@snapwich I discovered an issue around this line of code:
https://github.com/prebid/Prebid.js/blob/master/modules/prebidServerBidAdapter/index.js#L723

If the native adUnit does not have a sizes property defined, it gets filtered out by this function and is not included in the buildRequest() call. As a consequence, this result prevents an impression object from being made in ORTB request for that dropped adUnit.

Can you please take a look around this area of the code?

@snapwich
Copy link
Collaborator Author

@jsnellbaker I put in a check that allows native adUnits through prebid-server without sizes. I was just testing how other Native adUnits seemed to be working (by passing size [1,1]) but I think this is fine as well.

@snapwich snapwich merged commit f013970 into master Jul 30, 2019
@snapwich snapwich deleted the prebidServer-openrtb-native branch July 30, 2019 16:30
VideoReach pushed a commit to VideoReach/Prebid.js that referenced this pull request Aug 1, 2019
* initial support for native requests in prebid server

* add support for native request in prebid server OpenRTB

* fixes and new test for native openrtb responses

* updates to prebidServerBidAdapter for native support

* resolve conflicts with prebid-server video changes

* successfully returning and rendering native ad through prebid server

* add example prebid server native page

* fix bugs and tests for prebid-server native

* resolve unused variable lint alert in native example

* allow native adUnits without sizes in prebid server
@Austinb
Copy link

Austinb commented Sep 29, 2019

There is an issue with this code change and multiple banner size support in native ads in prebid.js version 2.34.0. In the example here http://prebid.org/dev-docs/show-multi-format-ads.html it shows support for multiple distinct sizes in the native.image.sizes property but when you use multiple distinct sizes the request format to a prebid server is wrong and the prebid server chokes on the input. Is this multiple size format below no longer supported/allowed for prebid native?

From the example page above:

native: {
    image: {
        sizes: [
            [728,90],
            [970,90]
        ]
    }
}

Becomes this example call into a prebid server with multiple sizes from prebis.js using a custom prebid server (latest version as of this comment's date):

{
  "id": "8c6253d3-fc62-4ede-a12b-4e03bed431d7",
  "source": {
    "tid": "8c6253d3-fc62-4ede-a12b-4e03bed431d7"
  },
  "tmax": 1000,
  "imp": [
    {
      "id": "/myunit/id",
      "ext": {
        "appnexus": {
          "use_pmt_rule": false,
          "placement_id": xxxxxx
        }
      },
      "banner": {
        "format": [
          {
            "w": 728,
            "h": 90
          },
          {
            "w": 970,
            "h": 90
          }
        ]
      },
      "native": {
        "request": "{\"context\":1,\"plcmttype\":1,\"eventtrackers\":[{\"event\":1,\"methods\":[1]}],\"assets\":[{\"required\":1,\"img\":{\"type\":3,\"w\":[728,90],\"h\":[970,90]}}]}",
        "ver": "1.2"
      }
    }
  ],
  "test": 0,
  "ext": {
    "prebid": {
      "targeting": {
        "includewinners": true,
        "includebidderkeys": false
      }
    }
  },
  "site": {
    "publisher": {
      "id": "1"
    },
    "page": "http://mysite.com"
  },
  "device": {
    "w": 2560,
    "h": 1329
  }
}

If you look at the native.request.assets section above you will see both the w and h properties are arrays of the defined sizes for the native request. At this point is where the prebid-server chokes with the following error Invalid request: json: cannot unmarshal array into Go struct field Image.w of type uint64. Removing the sizes from the native.image property allows the call to the prebid server to work as expected because that whole native.request section is no longer sent.

Also using a single size such as [1,1] works as expected because the values of the w and h properties in the request to the prebid server are both integers and not arrays. Example payload:

{
  "id": "782e7d90-dfdf-48b1-b546-e772fe9d123a",
  "source": {
    "tid": "782e7d90-dfdf-48b1-b546-e772fe9d123a"
  },
  "tmax": 1000,
  "imp": [
    {
      "id": "/myunit/id",
      "ext": {
        "appnexus": {
          "use_pmt_rule": false,
          "placement_id": xxx
        }
      },
      "banner": {
        "format": [
          {
            "w": 728,
            "h": 90
          },
          {
            "w": 970,
            "h": 90
          }
        ]
      },
      "native": {
        "request": "{\"context\":1,\"plcmttype\":1,\"eventtrackers\":[{\"event\":1,\"methods\":[1]}],\"assets\":[{\"required\":1,\"img\":{\"type\":3,\"w\":1,\"h\":1}}]}",
        "ver": "1.2"
      }
    }
  ],
  "test": 0,
  "ext": {
    "prebid": {
      "targeting": {
        "includewinners": true,
        "includebidderkeys": false
      }
    }
  },
  "site": {
    "publisher": {
      "id": "1"
    },
    "page": "http://mysite.com"
  },
  "device": {
    "w": 2560,
    "h": 1329
  }
}

From a quick debug it appears the issues happening at https://github.com/prebid/Prebid.js/blame/a7ad5ef0c7284147c0027cbf4317997542a232d7/modules/prebidServerBidAdapter/index.js#L512 where the code pulls the 0 and 1 indexes assuming the value being passed is [w,h] and does not take into account for [[w1,h1],[w2,h2]]. If multiple distinct sizes are no longer supported that's fine but the documentation needs to be updated to reflect that change.

@snapwich
Copy link
Collaborator Author

snapwich commented Oct 4, 2019

I believe this is an error in documentation. If you look at 4.4 of the OpenRTB Native Spec there is no ability for an image asset object to have more than one size, only w, h or a wmin, hmin.

@Austinb
Copy link

Austinb commented Oct 9, 2019

For multiple distinct sizes I assume we need to pass the [1,1] array moving forward then?

sa1omon pushed a commit to gamoshi/Prebid.js that referenced this pull request Nov 28, 2019
* initial support for native requests in prebid server

* add support for native request in prebid server OpenRTB

* fixes and new test for native openrtb responses

* updates to prebidServerBidAdapter for native support

* resolve conflicts with prebid-server video changes

* successfully returning and rendering native ad through prebid server

* add example prebid server native page

* fix bugs and tests for prebid-server native

* resolve unused variable lint alert in native example

* allow native adUnits without sizes in prebid server
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review needs 2nd review Core module updates require two approvals from the core team pinned won't be closed by stalebot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants