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

Showheroes update #4

Closed
wants to merge 18 commits into from
Closed

Showheroes update #4

wants to merge 18 commits into from

Conversation

FilipStamenkovic
Copy link

@FilipStamenkovic FilipStamenkovic commented Aug 13, 2024

Type of change

  • Updated bidder adapter <!-- IMPORTANT: (1) consider whether you need to upgrade your bidder parameter documentation in

Description of change

Update the showheroes-bs bidder adapter.

PR has following updates:

  • remove legacy code that is not supported anymore
  • replace the old endpoint with prebid-sh with new openrtb2/auction
  • use ortbConverter to construct bid request
  • in case of outstream ads, provide the renderer related parameters in the response (old static renderer is not working anymore)
  • by using openRTB, automatically add support for: fpd, floor module, eids, etc.

Other information

Copy link

Tread carefully! This PR adds 1 linter error (possibly disabled through directives):

  • modules/showheroes-bsBidAdapter.js (+1 error)

@musikele
Copy link

@FilipStamenkovic the file showheroes-bsBidAdapter.md should be reviewed and updated, because there are two sets of test parameters

Copy link

@musikele musikele left a comment

Choose a reason for hiding this comment

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

LGTM - no point in adding banner example ?

Copy link

@musikele musikele left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -271,98 +188,49 @@ function createBids(bidRes, reqData) {
bidUnit.vastUrl = bid.vastTag || bid.vastUrl;
}
if (bid.mediaType === BANNER) {
bidUnit.ad = getBannerHtml(bid, reqBid, reqData);
bidUnit.ad = getBannerHtml(bid, reqData);

Choose a reason for hiding this comment

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

I think we are mixing different things together here.

The previous banner implementation where based on the "legacy" ShowHeroes Player (pre viralize).

I don't really know if it's still up, I'm just seeing that https://bs.showheroes.com/api/v1/bid is returning 503 error.
As far as I remember that wasn't based on prebid server, thus I don't understand how can we use getBannerHtml with the current prebid server response.

I think we have to gather all the information about the legacy banner implementation and decide what to do in the new one.

Copy link
Author

Choose a reason for hiding this comment

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

We already have the banner implementation in the viralize repo: https://github.com/showheroes/viralize/blob/master/viralize/web/prebid.py#L43

I honestly don't know how it's working, but that part is unchanged. In our roadmap, we have to work on banner support for our bidder because, as far as I know, it currently works: it will just load the player and start the waterfall.

Choose a reason for hiding this comment

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

nobody is using the banner prebid and legacy showheroes is down. Let's remove the display part for now.

modules/showheroes-bsBidAdapter.js Outdated Show resolved Hide resolved
if (imp?.video && imp?.banner) {
delete imp['banner']
}
let mediaTypeContenxt = deepAccess(bidRequest, 'mediaTypes.video.context');

Choose a reason for hiding this comment

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

typo in the variable name

Copy link

Choose a reason for hiding this comment

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

thanks!

delete imp['banner']
}
let mediaTypeContenxt = deepAccess(bidRequest, 'mediaTypes.video.context');
if (!mediaTypeContenxt) {

Choose a reason for hiding this comment

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

We are choosing banner as the default context, however our banner integration might be broken (look at the comment below)

Copy link

Choose a reason for hiding this comment

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

I'll set VIDEO as default given that banner doesn't seem to be working

Comment on lines +64 to +66
(req?.device?.ua) && delete req.device['ua'];
// 'sua' is 2.6 standard, we operate with 2.5
(req?.device?.sua) && delete req.device['sua'];

Choose a reason for hiding this comment

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

are these two modifications needed?

Can they just be ignored server side?

Copy link
Author

Choose a reason for hiding this comment

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

I get it's premature optimization, but I just thought about not sending additional bytes of data for the things we are not using.

deepSetValue(ortbData, 'site.page', QA.pageURL);
const u = new URL(QA.pageURL);
deepSetValue(ortbData, 'site.domain', u.host);
ortbData.test = 1;

Choose a reason for hiding this comment

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

What is the effect of setting this property?

Normally on QA we perform e2e tests with settings very similar to production

Copy link

Choose a reason for hiding this comment

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

On adserver, we serve a rule that always returns an ad

Choose a reason for hiding this comment

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

What kind of ad?

If we want to performa a complete e2e test in qa, it will be impossible to try the prebid server with some real bidders?

const PROD_ENDPOINT = 'https://bs.showheroes.com/api/v1/bid';
const STAGE_ENDPOINT = 'https://bid-service.stage.showheroes.com/api/v1/bid';
const VIRALIZE_ENDPOINT = 'https://ads.viralize.tv/prebid-sh/';
const VIRALIZE_ENDPOINT = 'https://ads.viralize.tv/openrtb2/auction';

Choose a reason for hiding this comment

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

Let's rename the variable.

The duality between SH and viralize was introduced to have a single bidder handling two different integrations, one with the ShowHeroes legacy player, the other with the newly acquired Viralize.

Copy link

Choose a reason for hiding this comment

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

Renamed to SHOWHEROES_ENDPOINT, is it ok ?

Choose a reason for hiding this comment

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

Or just ENDPOINT

Comment on lines 14 to 17
const PROD_PUBLISHER_TAG = 'https://static.showheroes.com/publishertag.js';
const STAGE_PUBLISHER_TAG = 'https://pubtag.stage.showheroes.com/publishertag.js';
const PROD_VL = 'https://video-library.showheroes.com';
const STAGE_VL = 'https://video-library.stage.showheroes.com';

Choose a reason for hiding this comment

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

All this code might be obsolete if we will remove the legacy code (referring to banner)

Copy link

Choose a reason for hiding this comment

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

do we want to remove banner? for the info in my possession, we have very small traffic for it, but I am not aware of plans to dismiss banner

Choose a reason for hiding this comment

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

I think we need to dig deeper on how the current adapter works, I'm referring to the "legacy" stuff.

All the services behind showheroes.com domain are not belonging to MAX, they are considered "legacy" (video-library.showheroes.com static.showheroes.com).

Moreover, the current Max banner implementation was never delivered through the Prebid adapter.

bidUnit.currency = bid.currency;
bidUnit.mediaType = bid.mediaType || VIDEO;
bidUnit.ttl = TTL;
bidUnit.ttl = bid.exp || TTL;
bidUnit.creativeId = 'c_' + requestId;

Choose a reason for hiding this comment

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

Since we have completely changed the server returning the bidResponse, we should double check if all of these params are needed.
For example, is still needed to add prefix to creativeId ?
What is advertiserDomains ?

Copy link
Author

Choose a reason for hiding this comment

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

Most probably creativeId is not needed. My guess was it was needed for the previous renderer, but since that is no longer working it's not needed. But then again, on the other hand, we don't have a renderer in place, so at this point, we can't be really sure what is really needed and what isn't.

advertiserDomains is something each bidder must provide in response, it's a domain of advertisers delivering the ad.
It's being passed here: https://github.com/showheroes/viralize/blob/PLT-2267-adserver-add-dynamic-price-for-prebid-integration/viralize/web/executors/ad/auction.py#L131

@@ -250,12 +166,13 @@ function createBids(bidRes, reqData) {
let bidUnit = {};
bidUnit.cpm = bid.cpm;
bidUnit.requestId = requestId;
bidUnit.adUnitCode = reqBid.adUnitCode;
bidUnit.adUnitCode = bid.adUnitCode;
bidUnit.currency = bid.currency;
bidUnit.mediaType = bid.mediaType || VIDEO;

Choose a reason for hiding this comment

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

Here, contrary to what we did before, we are considering "VIDEO" as a fallback

@musikele musikele assigned musikele and unassigned FilipStamenkovic Sep 5, 2024

let script = loadExternalScript(urls.pubTag, 'showheroes-bs', function () {
window.ShowheroesTag = this;
const renderPayload = { ...response, ...renderConfig.renderOptions };

Choose a reason for hiding this comment

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

What type of object is response?
If I'm not wrong is something out of our direct control, coming from Prebid.js.
I'm wondering if it is ok to mix response and renderOptions, this might lead to some unwanted properties override.

Moreover it's not clear the contract between the renderer function and the argument it can receive.

Copy link
Author

Choose a reason for hiding this comment

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

I left like that on purpose. To be free to change/modify from the backend side whatever we want.
I didn't know what else to do. Right now we are serving appnexus renderer and we want to change that with showheroes renderer. So, to be able to control from the backend side without opening another PR in the pbjs repo.

I know it's not ideal, I know it's ugly and error-prone. But, until we don't have our renderer in place, I really don't see the other way. Then we can open a PR in pbjs and cleanup the mess that is left behind.

Another option is to hardcode appnexus renderer inside pbjs, and then 2nd PR to replace it with ours. Code would be a lot cleaner. Let me know what you think?

Choose a reason for hiding this comment

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

I was just looking at the ANOutstreamVideo.renderAd arguments and how other bidders are using it (simplest one).

Maybe it's better to explicitly return the object passed to the renderer directly from the server response

function outstreamRender(response, renderConfig) {
  response.renderer.push(() => {
    const func = deepAccess(window, renderConfig.renderFunc);
    if (!isFn(func)) {
      return;
    }
    func(renderConfig.renderOptions);
  });
}

This means that adserver must be changed with:

if media_type == settings.PREBID_FORMAT_OUTSTREAM:
    openrtb_bid.ext["rendererConfig"] = {
        "rendererUrl": RENDERER_URL,
        "renderFunc": "ANOutstreamVideo.renderAd",
        "renderOptions": {
            "showBigPlayButton": False,
            "showProgressBar": "bar",
            "skippable": False,
            "targetId": adunit_code,
            "adResponse": {
                "content": winning_xml
            }
        },
    }

}
const responseBids = bidRes.bids || bidRes.bidResponses;
if (!Array.isArray(responseBids) || responseBids.length < 1) {
function createBids(bidRes, bidReq) {

Choose a reason for hiding this comment

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

As discussed in private conversation, we'd like to use (if possible) converter.fromORTB by following the docs

Copy link
Author

Choose a reason for hiding this comment

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

It is possible, but unfortunately, we have many customizations that I would have to extend the code and I wouldn't get any specific benefit. We have:

  • extra param (that I think it's going to be needed for the player later on)
  • callbacks that are fired when our bidder wins the auction
  • renderer specific items

Copy link
Author

Choose a reason for hiding this comment

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

aaah, I jumped the gun and responded before turning on my brain. Actually using the fromORTB can remove 20 lines of code, which is great. I'll update the code

bidResponse(buildBidResponse, bid, context) {
const bidResponse = buildBidResponse(bid, context);

if (context.imp?.ext?.mediaType === 'outstream') {

Choose a reason for hiding this comment

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

This line looks suspicious, maybe you wanted to write:

if (bidResponse.mediaType === VIDEO && bidRequest.mediaTypes.video.context === 'outstream') {


let script = loadExternalScript(urls.pubTag, 'showheroes-bs', function () {
window.ShowheroesTag = this;
const renderPayload = { ...response, ...renderConfig.renderOptions };

Choose a reason for hiding this comment

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

I was just looking at the ANOutstreamVideo.renderAd arguments and how other bidders are using it (simplest one).

Maybe it's better to explicitly return the object passed to the renderer directly from the server response

function outstreamRender(response, renderConfig) {
  response.renderer.push(() => {
    const func = deepAccess(window, renderConfig.renderFunc);
    if (!isFn(func)) {
      return;
    }
    func(renderConfig.renderOptions);
  });
}

This means that adserver must be changed with:

if media_type == settings.PREBID_FORMAT_OUTSTREAM:
    openrtb_bid.ext["rendererConfig"] = {
        "rendererUrl": RENDERER_URL,
        "renderFunc": "ANOutstreamVideo.renderAd",
        "renderOptions": {
            "showBigPlayButton": False,
            "showProgressBar": "bar",
            "skippable": False,
            "targetId": adunit_code,
            "adResponse": {
                "content": winning_xml
            }
        },
    }

Copy link

@francesconistri francesconistri left a comment

Choose a reason for hiding this comment

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

Remember to change the adserver implementation about the video context

Copy link

@musikele musikele left a comment

Choose a reason for hiding this comment

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

LGTM

@FilipStamenkovic
Copy link
Author

FilipStamenkovic commented Sep 30, 2024

Official PR opened.

@FilipStamenkovic FilipStamenkovic deleted the showheroes_update branch September 30, 2024 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants