-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Experimental Fledge Module & OpenXOrtb Adapter: initial release and bidder support #8716
Conversation
This pull request introduces 4 alerts when merging 8be2ee1 into cb7c510 - view on LGTM.com new alerts:
|
Co-authored-by: Laurentiu Badea <laurentiu.badea@openx.com> Co-authored-by: Luigi Sayson <luigi.sayson@openx.com> Co-authored-by: Kenan Gillet <kenan.gillet@openx.com>
8be2ee1
to
16103ac
Compare
This pull request introduces 1 alert when merging 16103ac into cb7c510 - view on LGTM.com new alerts:
|
Unfortunately, regardless of value of the |
@brodrigu are you able to arrange a reviewer? |
thank you piotrj-rtbh
thanks, seems to be unrelated as it looks like the response.body is null #8725 should avoid the unnecessary logging |
Hey @kenan-gillet and @patmmccann - I have a few questions in order to review this PR; keep in mind that I've not yet looked at the code. I first have to understand what is Fledge exactly.
|
Yes
It is meant to be merged, I think POC is referring to the fact that Fledge itself is still in a POC state with origin trials live.
I'm not sure, the main functionalities are detection of a fledge eligible auction (likely to occur on chrome 104 with privacy sandbox opted into), passing of the auctionconfig from the bidder to the module, and submission of auction config to an external api. I suppose focus may be on confirming all of those seem to operate roughly as expected. However, it seems @brodrigu has kindly contributed comments; I would be comfortable taking his review as the second review if he is as well. |
``` prebid.js:5 Prebid openx ERROR: Bidder openx failed to interpret the server's response. Continuing without bids null TypeError: Cannot use 'in' operator to search for 'nbr' in at prebid.js:12:4571 ``` Thanks @piotrj-rtbh
- make fledgeManager into a module - add top-level auction handling - add support for registering component auction to GPT ad slot - remove hard coded demo auction config, ensure auctionSignals is not undefined (prebid#61) Co-authored-by: Laurentiu Badea <laurentiu.badea@openx.com> Co-authored-by: Luigi Sayson <luigi.sayson@openx.com> Co-authored-by: Kenan Gillet <kenan.gillet@openx.com>
modules/fledge.js
Outdated
const MODULE = 'fledge secure'; | ||
const fledgeAuctionConfig = getAuctionConfig(adObject); | ||
if (fledgeAuctionConfig) { | ||
logWarn(MODULE, `Ad id ${adObject.adId} is being fledged`, fledgeAuctionConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are warnings appropriate here? it doesn't appear like anything went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These merely indicate our interest in seeing that particular message when it happens during development.
We will change to more appropriate logging levels.
modules/fledge.js
Outdated
return targetAdUnitCode == adUnitCode; | ||
}); | ||
if (componentAuctionsForAdUnitCode.length == 0) { | ||
logInfo(MODULE, `NOT running runAdAuction for adUnitCode=${targetAdUnitCode} since there are no componentAuctions`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conversely this sounds like a warning to me :)
modules/fledge.js
Outdated
|
||
export function getAuctionConfig(bid) { | ||
const targetAdUnitCode = bid.adUnitCode; | ||
const componentAuctionsForAdUnitCode = _componentAuctions.filter((componentAuction) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can run an auction with multiple adUnits that have the same code, you can also run multiple auctions on the same adUnits. Does this approach work for those cases?
bids have a transactionId
that might work better as a filter. They are unique for each auction-adUnit pair, and you should have their association with the fledge configs through their bidId
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adUnitCode
is needed to find the GPT slot associated with the adUnit. With GAM, which may well be most of the use cases, the fledge auction config is set on the GPT slot and GPT triggers the auction.
When GAM isn't in use, the configs could be organized a different way, true, but this depends on what the buyers expect to bid on and those bidding functions would then need to take into account the whole environment they bid in.
Would you happen to know the use case for sharing adUnitCode
across different adunits ? Is this a common case ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even when using GAM, it looks easier to me to filter by transactionId. If you don't you'll have to filter by adUnitCode and auctionId, and that does not cover the repeated adUnitCode case.
modules/fledge.js
Outdated
} | ||
|
||
// fallback if there is no top-level seller defined | ||
const randomIdx = Math.floor(Math.random() * componentAuctionsForAdUnitCode.length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this makes sense, won't it result in fewer / lower bids overall, since it excludes the original winning bid (that may be higher than fledge's)? to me it seems like a top level auction managed by Prebid is required to avoid throwing that away, I'm not sure that it even makes sense to allow configurable seller / decision logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The expectation for a correctly configured publisher is for the decisionLogicUrl
to be defined and the "multiple ssp with top level support" path be taken. The decision logic script will likely be provided by prebid as a common resource in some way.
Due to the special requirements for serving it*), it cannot be bundled in, so for the moment we made it configurable.
*) the response header x-allow-fledge: true
must be passed when serving the decision logic scripts. It also has to come from the same domain as the seller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's reasonable, but if no decision logic is available, to me it'd seem better to discard fledge rather than awarding the auction to a random fledge bidder.
modules/fledge.js
Outdated
logWarn(MODULE, `Ad id ${adObject.adId} was NOT fledged, rendering normal ad`); | ||
} | ||
// we use the default secure rendering | ||
next(reply, data, adObject, {isFledge: true}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think isFledge
here should be true only if you got a fledgeAdUri
back from navigator.runAdAuction
.
modules/fledge.js
Outdated
|
||
export function runAdAuction(bid, next, doc, id, options) { | ||
if (bid.mediaType == 'video') { | ||
logWarn(MODULE, 'Video fledge ads not yet supported.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think native would work either, since it can't be reduced to a single URL.
modules/fledge.js
Outdated
if (fledgeAdUri) { | ||
logWarn(MODULE, `Ad id ${adObject.adId} has been fledged by ${fledgeAdUri}`); | ||
adObject.ad = ''; | ||
adObject.adUrl = fledgeAdUri; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
most other things in adObject would be incorrect, and I'm worried that will cause things to break - in fact I think that's inevitable in the long run. Is it feasible to create a new object here, or would that force you to still pick incorrect values? Maybe it would be better to run the fledge auction from the creative directly.
hello, Also, we will create a separate PR to continue working on prebid running the fledge auction and rendering the winning fledge ads if people are still interested. |
@bwschmidt would appreciate your review as second review here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You were not kidding, this is a lot simper. The linter is complaining right now, but if once fixed tests pass this LGTM.
@patmmccann @kenan-gillet the module itself lgtm. Enablement does not seem clear to me. The readme indicates enabling fledge support for the adunitortb2Imp.ext.ae must be 1 ortb2Imp: {
ext: {
ae: 1
}
} however i do not see this actually referenced in the code. Is fledge meant to be enabled globally or per ad slot? I think its working globally per bidder now and the openx adapter is doing this
i think that is fine i would just make the readme more clear |
thanks @bwschmidt , I made clearer that it is needs to be enabled at 3 different levels |
…idder support (prebid#8716) * Fledge POC: adding run ad auction and rendering with openxOrtbBidAdapter Co-authored-by: Laurentiu Badea <laurentiu.badea@openx.com> Co-authored-by: Luigi Sayson <luigi.sayson@openx.com> Co-authored-by: Kenan Gillet <kenan.gillet@openx.com> * Remove defensive coding: gtpSlotInfo is always an object * Change onBidderAuctionResponse callback to specialized onFledgeAuctionConfigs * openxOrtbBidAdapter: avoid error logging when response body is null ``` prebid.js:5 Prebid openx ERROR: Bidder openx failed to interpret the server's response. Continuing without bids null TypeError: Cannot use 'in' operator to search for 'nbr' in at prebid.js:12:4571 ``` Thanks @piotrj-rtbh * Fledge Modularized - make fledgeManager into a module - add top-level auction handling - add support for registering component auction to GPT ad slot - remove hard coded demo auction config, ensure auctionSignals is not undefined (prebid#61) Co-authored-by: Laurentiu Badea <laurentiu.badea@openx.com> Co-authored-by: Luigi Sayson <luigi.sayson@openx.com> Co-authored-by: Kenan Gillet <kenan.gillet@openx.com> * Split GPT handling into its own module * Remove fledge run/rendered by prebid * Fix lint issues and name inconsistency * Update unit test after switching to GPT only * fix/remove no more relevant tests * fledgeForGpt: clarify documentation of configuration Co-authored-by: Laurentiu Badea <laurentiu.badea@openx.com> Co-authored-by: Luigi Sayson <luigi.sayson@openx.com>
…idder support (prebid#8716) * Fledge POC: adding run ad auction and rendering with openxOrtbBidAdapter Co-authored-by: Laurentiu Badea <laurentiu.badea@openx.com> Co-authored-by: Luigi Sayson <luigi.sayson@openx.com> Co-authored-by: Kenan Gillet <kenan.gillet@openx.com> * Remove defensive coding: gtpSlotInfo is always an object * Change onBidderAuctionResponse callback to specialized onFledgeAuctionConfigs * openxOrtbBidAdapter: avoid error logging when response body is null ``` prebid.js:5 Prebid openx ERROR: Bidder openx failed to interpret the server's response. Continuing without bids null TypeError: Cannot use 'in' operator to search for 'nbr' in at prebid.js:12:4571 ``` Thanks @piotrj-rtbh * Fledge Modularized - make fledgeManager into a module - add top-level auction handling - add support for registering component auction to GPT ad slot - remove hard coded demo auction config, ensure auctionSignals is not undefined (#61) Co-authored-by: Laurentiu Badea <laurentiu.badea@openx.com> Co-authored-by: Luigi Sayson <luigi.sayson@openx.com> Co-authored-by: Kenan Gillet <kenan.gillet@openx.com> * Split GPT handling into its own module * Remove fledge run/rendered by prebid * Fix lint issues and name inconsistency * Update unit test after switching to GPT only * fix/remove no more relevant tests * fledgeForGpt: clarify documentation of configuration Co-authored-by: Laurentiu Badea <laurentiu.badea@openx.com> Co-authored-by: Luigi Sayson <luigi.sayson@openx.com>
Type of change
Description of change
Allow running/rendering fledge ad when prebid ad wins
Other information
demo is at https://codinginadtech.com/prebid-live-tests/fledge/?pbjs_debug=true&test=true&use_test_ad=true&fledge_enabled=true
Co-authored-by: Laurentiu Badea laurentiu.badea@openx.com
Co-authored-by: Luigi Sayson luigi.sayson@openx.com
Co-authored-by: Kenan Gillet kenan.gillet@openx.com