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

Added new types of traffic Colossus SSP adapter #2281

Merged
merged 5 commits into from
Apr 26, 2018

Conversation

HuddledMasses
Copy link
Contributor

Type of change

  • Feature

Description of change

Added new types of traffic Colossus SSP adapter.

  • test parameters for validating bids
{
   bidder: 'colossusssp',
   params: {
      placement_id: 0,
      traffic: 'video'
    }
}

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.

Hello @HuddledMasses ,

If you are planning to support video & native, the colossus SSP bidder spec should include

supportedMediaTypes: ['banner', 'video', 'native'],

(below aliases or anywhere in the spec is fine). See this note in the docs for more info on supporting video & native (if you haven't already).

Additionally, can you please also remove the changes to the package-lock.json file from this PR? Thanks!

@HuddledMasses
Copy link
Contributor Author

Hi @jsnellbaker
here is the updated Colossus docs pull request: #680

@jsnellbaker
Copy link
Collaborator

Hi @HuddledMasses I'm a bit confused, you are planning to support banner, video and native - right?

If so, the docs PR only included the banner support. Shouldn't it also note the video & native support? Also, did you have the chance to look at the recommendations for this PR?

Thanks.

@HuddledMasses
Copy link
Contributor Author

HuddledMasses commented Mar 26, 2018

We updated pull request.
Please, review.
@jsnellbaker

@jsnellbaker
Copy link
Collaborator

Hi @HuddledMasses , thanks for submitting the update. I was trying to test the adapter with the test params included in the PR (for the video ad unit?), but the adapter code wasn't accepting the bid response.

It seems like a banner type unit came back instead of a video type. Further the bid was rejected because it didn't contain a mediaType object in the response, which is needed in the isBidResponseValid() check.

Can you take a look at the test param information/bid response?

Also, if you're going to now support additional ad unit types, I'd like to test each type (banner, video, native) to ensure they're all working successfully with your updated adapter. If you can provide test param info for each ad type, I'd greatly appreciate it.

For reference here is a copy of the bid response I received:
[{"width":300,"height":250,"ad":"<img src='http://supply.huddledmasses.com/img/logo.png' style='position: absolute; left: 50%; top:50%; margin-left: -113px; margin-top:-79px;'>","requestId":"28419cc0d6c5ca","cpm":5,"ttl":120,"creativeId":"123","netRevenue":true,"currency":"USD","dealId":"HASH"}]

@HuddledMasses
Copy link
Contributor Author

Hi @jsnellbaker
I updated the server.
Added test placements.
Banner: {placement_id:0, traffic:"banner"}
Video: {placement_id:0, traffic:"video"}
Native: {placement_id:0, traffic:"native"}
Can you please check?

@jsnellbaker
Copy link
Collaborator

Hi @HuddledMasses,
Thanks for providing the additional test ad units and making the change on your server.

I tested out each of the ad units; see details/feedback below:

  • banner: LGTM
  • video: LGTM
  • native: needs some additional changes (details below).

When we're receiving a native bid response, certain properties of the native bid response need to be defined in the bid under a native object. These properties are largely the native ad's assets (like image (with url, height, width), clickurl, impressionTrackers, etc). If these properties are not included, the bid is rejected (which is what I was seeing while testing).

The following page has a sample markup for the bid.native object that needs to be populated, as well as some additional information.
http://prebid.org/dev-docs/bidder-adaptor.html#supporting-native

Can you take a look and make the changes? I was using the hello_world.html page for this testing if you want check it on your end as well. Please let me know if you have any questions. We should be good once this last piece is in place and working.

@HuddledMasses
Copy link
Contributor Author

Hi @jsnellbaker
We changed native response validation.
Also changed the formation of the response from the server.
The documentation indicates the ability to set different types of traffic.
https://github.com/prebid/prebid.github.io/blob/master/dev-docs/bidders/colossusssp.md

@jsnellbaker
Copy link
Collaborator

jsnellbaker commented Apr 10, 2018

Hi @HuddledMasses thanks for submitting the changes. Unfortunately, it doesn't seem like the native bid response is being accepted by the internal validation.

The issue seems to boil down to where the native information (clickurl, image object) is being stored. It's currently being added to the bid overall, instead into a native object within the bid.

As an example, this is the current appearance of the bid response object as populated from the adapter:

  "bidderCode": "colossusssp",
  "width": 0,
  "height": 0,
  "statusMessage": "Bid available",
  "adId": "201d8dea935e26",
  "mediaType": "native",
  "source": "client",
  "clickUrl": "http://supply.colossusssp.com",
  "title": "Test",
  "image": {
    "url": "http://supply.colossusssp.com/img/logo.png",
    "width": 226,
    "height": 158
  },
  "impressionTrackers": [
    "colossusssp.com"
  ],
  "ttl": 120,
  "cpm": 0.4,
  "requestId": "201d8dea935e26",
  "creativeId": "2",
  "netRevenue": true,
  "currency": "USD",
  "dealId": "1"
}

It should ideally store the native related information like this:

{
  "bidderCode": "colossusssp",
  "width": 0,
  "height": 0,
  "statusMessage": "Bid available",
  "adId": "201d8dea935e26",
  "mediaType": "native",
  "source": "client",
  "native": {
     "clickUrl": "http://supply.colossusssp.com",
     "title": "Test",
     "image": {
        "url": "http://supply.colossusssp.com/img/logo.png",
        "width": 226,
        "height": 158
      },
      "impressionTrackers": [
          "colossusssp.com"
       ],
   }, 
  "ttl": 120,
  "cpm": 0.4,
  "requestId": "201d8dea935e26",
  "creativeId": "2",
  "netRevenue": true,
  "currency": "USD",
  "dealId": "1"
}

Can you try to make the necessary changes? Please let me know if you have any questions.

@HuddledMasses
Copy link
Contributor Author

Hi @jsnellbaker
We have made changes in the native.
Also added the transfer of data by size to the server.

@jsnellbaker
Copy link
Collaborator

Hi @HuddledMasses, Thanks again for making the changes.

I tested the native ad unit and found it was structured properly and passing the internal checks. However, ad unit wasn't rendering on the test page. While looking over the browser console, I noticed the pbjs.renderAd function was being executed after we got the bid response.

This is normally used to help render banner and video ads and is invoked from the prebid creative template. The native creative template is different (as it has to handle different creative assets).

The following page gives an outline of what the native creative code looks like: http://prebid.org/adops/setting-up-prebid-native-in-dfp.html

Can you take a look over this page and try to make the needed changes? I believe we should be good to merge after this is addressed. Thanks!

@HuddledMasses
Copy link
Contributor Author

@jsnellbaker
I don't understand.
Where do I need to make changes?
The adapter doesn't affect the operation or sequence of the function call pbjs.renderAd.

@jsnellbaker
Copy link
Collaborator

@HuddledMasses There should be a creative in your ad server that would return a 'prebid' creative. Normally this creative is a set of JavaScript that makes the call the pbjs.renderAd to render a video or banner creative that's retruned from the auction. In this case for the native, the creative should be setup with a different creative template in the format that was referenced in the page from before (ie plain HTML).

Maybe one of the traffickers or adops people on your end may be able to help point you to the area where the changes need to be made?

If it will help, below is a link to a more parent-level page on the different ways to setup prebid creatives:
http://prebid.org/adops/docs-by-ad-server.html

@HuddledMasses
Copy link
Contributor Author

@jsnellbaker
I don't understand.
What returns the adapter is not correct ? If so you can say what is wrong.
Or a template must be created. If so, then where to add it ?

@jsnellbaker
Copy link
Collaborator

@HuddledMasses I'm sorry about the confusion on the last point in this PR. I conferred with a colleague and got this straightened out. It seems everything is fine at this point. I'm going to merge in this PR.

@jsnellbaker jsnellbaker merged commit 5c14e9c into prebid:master Apr 26, 2018
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
* add video&native traffic colossus ssp

* Native obj validation

* Native obj validation #2

* Added size field in requests

* fixed test
@HuddledMasses
Copy link
Contributor Author

Team, can you please let us know why Colossus adapter has not been listed here?

http://prebid.org/dev-docs/bidders.html#bidders-with-video-and-native-demand

@jsnellbaker
Copy link
Collaborator

Hi @HuddledMasses

To have the bidder appear on that table, you'd need to submit a PR to the Prebid Docs repo (located here) to update this file:
https://github.com/prebid/prebid.github.io/blob/master/dev-docs/bidders/colossusssp.md

Just need to add the following variables to the top section (where the other variables are located):

media_types: banner, video, native

@HuddledMasses
Copy link
Contributor Author

HuddledMasses commented Sep 16, 2019

Hi @jsnellbaker
PR has been updated: prebid/prebid.github.io#1484

Please take a look. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants