Prebid Core: creative comment injection spot reverted

[denys-berzoy-confiant]

This PR is mirroring the changes in #7874 to the v5.x release, which a lot of our clients still use.
So, the original PR's comments below:

• reverted injection point of creative comment to pre-PR Inject creativeComment after render so it actually persists #6860
• added code to reinject comment in case it was removed upon rendering

Type of change

• Bugfix

Description of change

This is an adjustment to the code previously pushed (#6860) to address the issue of disappearing bid detail comments in the document. We (Confiant) as a provider of ad quality and security need to have access to the bid detail comment before rendering starts, to match it against our data. The previous push broke our logic. We would like to put the injection of the comment back to the pre-rendering stage and only re-inject it again if it was in fact removed by the adaptor rendering as noticed by the previous developer (@jsut).

[denys-berzoy-confiant]

Ugh...

Chrome 79.0.3945.79 (Windows 10) -> PASS
Chrome 90.0.4430.72 (Windows 10) -> 1 FAIL

Putting aside question regarding testing against actual v97, but is there any way for me to debug that one?

[dgirardi]

@denys-berzoy-confiant we have some tests that fail spuriously, could you try to "touch" this pr to run tests again? (git commit --amend -C HEAD && git push -f)

I am collecting these failures to try and debug them - it's likely not related to this PR.

[dgirardi]

LGTM, especially since it's been approved already, but I'd like to understand the use case a bit more - to me it seems there has to be a better way to do what you're doing with this.

Consider that renderAd is not the only way ads are rendered. (this is another way, or this). If in your case you know ads will always go through renderAd, maybe that means Prebid needs more ways to customize that?

[denys-berzoy-confiant]

@dgirardi well, for ad quality purposes we match the creative ids against the blocking rules that a publisher selects to have control over the ads, so we need the creative id before display ad renders. We are aware of other ways to render ads you mentioned, it's just this one workflow got broken and we're simply restoring it.

[dgirardi]

@denys-berzoy-confiant would it make sense to block certain creative IDs during the auction, so that an alternative ("approved") ad can be rendered instead? I believe this is possible right now with bidCpmAdjustment although that's not what it's meant for. (@bretg, have you seen similar use cases?)

[jsut]

@denys-berzoy-confiant would it make sense to block certain creative IDs during the auction, so that an alternative ("approved") ad can be rendered instead? I believe this is possible right now with bidCpmAdjustment although that's not what it's meant for. (@bretg, have you seen similar use cases?)

From the publisher point of view, filtering a bid from an 'invalid' bidder, so they can't win an auction is a far better solution. It could mean the difference in getting paid on an auction, vs not getting paid.

[bretg]

If this is so important, it needs comments explaining to future developers what it's for so they don't (re)remove it.

[bretg]

another thought: why don't you guys have a module that taps into the bidresponse hook?

[denys-berzoy-confiant]

@jsut @bretg @dgirardi well, first of all, as it was said, bid cpm adjustment API was not designed for this stuff. Secondly, it would be more like using a hammer against cockroaches. Our solution allows more fine-grained filtering based not only on bidders, but the specific creative's ID. We then send it upstream to SSPs for them to remove it, but since it's manual process for them it takes some time, while we take action immediately across the network of our clients.

In server-to-server bidding there's bcat and badv specs from RTB to store publisher block filters which block by category or certain advertiser domains, but as far as I know, this is not implemented in Prebid:

bcat string array Blocked advertiser categories using the IAB content categories. Refer to List 5.1.
badv string array Block list of advertisers by their domains (e.g., "ford.com").

https://www.iab.com/wp-content/uploads/2016/03/OpenRTB-API-Specification-Version-2-5-FINAL.pdf

PS: added a comment on the code

[dgirardi]

@denys-berzoy-confiant I agree working on CPM is not pretty, but working on the rendered DOM is worse :) the creativeId is part of the bid and could be filtered out before the auction is done, which would have the advantage @jsut mentioned. Filtering on domain should also be possible in principle but I think that'd be more convoluted right now. IMO this issue shows Prebid needs better docs / support for filtering bids getting to the auction, the examples we have are for the most part about modifying them.

[jsut]

I agree working on CPM is not pretty, but working on the rendered DOM is worse :)

100%

In the comments on the original PR (now merged in master) I noted that there is nothing that will stop someone from changing this again, and breaking Confiant's code. They should not have a product that relies on an undocumented piece of 5 year old debug code #2158. This is just asking for trouble. I have literally no say as I'm not even a prebid.org member, but IMO the path forward here should be something like this:

1. Confiant creates an issue on prebid.js about the problems with hooking bidResponse or using bidCPMAdjustment to do their thing, vs parsing the DOM, so prebid members and Confiant can come up with a better, supported integration plan.
2. This PR gets merged.
3. Confiant implements whatever the solution the issue yields, with support from prebid.org as required.

I'm tempted to suggest that this comment should only be injected if pbjs.debug is enabled, but it's far too useful for stuff like identifying malicious ads etc, and I do think prebid.js should inject something like this for that purpose. But nothing should actually rely on it's presence, format, or content.

[bretg]

We need another reviewer -- my approval is just that there's an appropriate comment.