-
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
Weborama RTD Module : start Bidder specific handling removal #10005
Weborama RTD Module : start Bidder specific handling removal #10005
Conversation
} else { | ||
logMessage(`SKIP bidder '${bid.bidder}', data from '${metadata.source}' is not defined as user or site-centric`); | ||
} | ||
// this.#setBidderOrtb2(reqBidsConfigObj.ortb2Fragments?.bidder, bid.bidder, base, profile); |
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.
So just to clarify for appnexus - we can read data from the various ortb2 keyword fields (we'll pass the data into our existing keywords fields), but we can't read from the segment related ortb data fields at this time. I have checked with our product team and there's no current home for all of the related ortb segment fields of data in our current endpoint (note - there is some ongoing discussions on this topic, so there might be different news at some point down the road).
I hope this helps clarify what type of code you could change or leave in place.
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.
hey, I try to handle it better in the last next two commits
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.
we can't read from the segment related ortb data fields at this time.
@astephensxandr if this is indeed true for the foreseeable future, Microsoft should expect disruption in its ongoing compatibility with RTD modules. Are you saying you guys are comfortable with this? The current situation is in order to avoid a small amount of work by microsoft -- specifically figuring out some translation of fields in user.ext.data into your keywords object -- you are pushing quite difficult work onto many of your partners, not all of which have been able to achieve it.
modules/weboramaRtdProvider.js
Outdated
} else { | ||
logMessage(`SKIP bidder '${bid.bidder}', data from '${metadata.source}' is not defined as user or site-centric`); | ||
this.#assignProfileToObject(bid, 'params.keywords', profile); |
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'm trying to follow the updates that were made in this area with the recent commits.
If this code is still primarily there for the appnexus bid adapter (as implied by the LEGACY_SITE_KEYWORDS_BIDDERS
if check made earlier to call this handleSiteLegacyKeywordsBidders
function), you could move it or generalize it further by adding this data to the equivalent ortb2 fields (like site.keywords) instead of the bidder.params location. Our adapter should be able to read that data from the ortb2 location, and it should allow other bidders to read that data as well.
If this is serving some other purpose still, can you please clarify? Thanks.
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.
Hello
I just copy this LEGACY_SITE_KEYWORDS_BIDDERS
from 1plusXRtdProvider.js
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'm not sure I follow - the 1plusXRtdProvider appears to be writing a modified form of segment data into the appnexus auction keywords field (which they technically don't need to do anymore, since they could write it to the ortb2.site.keywords field instead).
So while I may understand if you want to do an extra step/conversion of your data to a keyword-like format (like 1plusXRtdProvider), you shouldn't need to target the bidder keywords field like they do.
The only reason I'm bringing up this point was that the PR seemed to be designed to move your data to the ortb2 fields. So that should be possible even for us (appnexus), instead of using a bidder param field (which feels like it's against the grain).
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.
so if I understand well, in the case of appnexis I must push to ortb2.site.keywords
or ortb2.user.keywords
depending if it is site-centric or user-centric, right?
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.
For our endpoint, we just have either request level keywords or adunit/placement specific keywords.
For our request level keywords they read the ortb2 keyword fields from various sources, collating them together based on keynames/keyvalues to send along in the auction request. For the adunit keywords, we read from the adUnit ortb2Imp field that publishers can choose to populate, if they have unique data for that slot. So it doesn't as much matter which of the fields are used in the ortb2 keywords properties, we'll check them and pull in the data if it's there.
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.
how about now: db1fc08 ?
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.
There are two factors that should be reviewed:
- the data is getting set as an object in the code under the site.keywords, but according to the ortb spec, the data for a keywords field should be a comma separated list of keypairs (like
genre=rock,genre=pop,pets=dog
). If it's an object format, bidders may not read it correctly. - secondly when I went to test the changes using the
integrationExamples/gpt/weboramaRtdProvider_example.html
test page, the site.keywords field was not present in the bid request's ortb2 object. I tried to step through the code in the weboramaRtdProvider and it seemed to attach the keywords to the requests object, but then it got lost later for some reason?
I attached screenshots for reference.
weborama debug
appnexus bid adapter debug
overall bid request from console logs
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.
#10001 just merged and might be helpful, thanks so much! |
modules/weboramaRtdProvider.js
Outdated
case 'rubicon': | ||
this.#handleRubiconBid(bid, profile, metadata); | ||
break; | ||
if (bidder === APPNEXUS_BIDDER) { |
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.
To summarize what I think is missing:
- appnexus (and clones) now parse keywords as
key=value
pairs. so this should serialize objects like{ webo_ctx: ["A", "B"], webo_ds: ["C", "D"]}
down to a single string such aswebo_ctx=A,webo_ctx=B,webo_ds=C,webo_ds=D
.
- because appnexus has clones, this should ideally not check for
appnexus
specifically. You could take the list of bidders to set keywords for as config, or you could even set these keywords on all the bidders you want to set segments for (non-appnexus bidders will probably have no use for them but they shouldn't hurt).
Hello Since there is a lot of changes that may be added on this MR, I decided to split the development in 3 parts
I just rollback the last 4 commits. Thanks a log guys |
this.#handleRubiconBid(bid, profile, metadata); | ||
break; | ||
if (bidder == 'appnexus') { | ||
this.#handleAppnexusBid(reqBidsConfigObj, bid, profile); |
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.
would you prefer to remove this line, move the handling into the appnexus adapter, or just not be in 8 until this is resolved?
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.
@peczenyj update here! Appnexus pr should make this much easier, we're delaying prebid 8 to accommodate
@peczenyj This PR achieves your first goal; thanks so much! We do plan to release prebid 8 imminently however and need all specific handling removed for you to be in it. Please let us know if you prefer us to remove your adapter or your appnexus handling. Update: appnexus adapter should make this much easier! |
* master: Colossus Bid Adapter: gpp support (prebid#10096) Increment version to 8.1.0-pre Prebid 8.0.0 release Prebid 8: initial release (prebid#10071) Increment version to 7.54.1-pre Prebid 7.54.0 release Update topicsFpdModule.js (prebid#9990) Added GPP support (prebid#10094) Teads Bid Adapter:Remove auctionId from since it is not used anymore (prebid#10098) HypeLab Bid Adapter: Initial Release (prebid#10003) Oxxion Rtd Module: listen onBidResponseEvent instead of onAuctionEndEvent (prebid#10083) PubMatic Analytics Adapter : Added extra fields in tracker for analytics (prebid#10090) AdHash Bidder Adapter: update for brand safety (prebid#10087) videojsVideoProvider: use contrib-ads event names (prebid#10081) Increment version to 7.54.0-pre Prebid 7.53.0 release Add bidgency alias (prebid#10077) Weborama RTD Module : start Bidder specific handling removal (prebid#10005) ZetaGlobalSspBidAdapter: change logic around mediaType (prebid#10074) add tmax to sovrnBidAdapter (prebid#10059) Mediasquare Bid Adapter: Avoid to run bidwon event on video impressions (prebid#10068) FreePass User ID Module : initial release (prebid#9814) Conceptx Bid Adapter : initial release (prebid#9835) beOp BidAdapter: FirstPartyData management and ingest Permutive segments (prebid#9947) StroeerCore Bid Adapter: add price floor support (prebid#9962) YieldlabBidAdapter updated native asset mapping (prebid#9989) Mediasquare Bid Adapter: handle context for video bids (prebid#10055) Add new alias - Apester (prebid#10057) Criteo Bid Adapter: Pass outstream video renderer method (prebid#10054) Add: Viously GVL ID (prebid#10061) 1plus x RTD Adapter: remove bidder specific handling enforcement (DC 3634) (prebid#10001) Sirdata RTD Module : bidder specific handling removal (prebid#9970) feat: pass video.plcmt [PB-1736] (prebid#10058) Update adkernelBidAdapter.js (prebid#10056) Eskimi Bid Adapter: Added support for video (prebid#9985) Richaudience Bid Adapter : add compability with GPID (prebid#9928) Add GVL ID for pairId userId submodule. (prebid#10053) Update nexx360BidAdapter.js (prebid#10042) Logicad Bid Adapter: Add topics and uach support (prebid#10045) change session_id fallback to bidderRequestId (prebid#10047) ZetaGlobalSsp Bid Adapter : process array of sizes (prebid#10039) AdagioBidAdapter: change outstream renderer (prebid#10035) update greenbids analytics endpoint (prebid#10037) ID5 User Id module - get whole ext object from server response (prebid#10036) Criteo Bid Adapter : Bump fastbid version to 136 (prebid#9973) UID2 & EUID Modules: Add support for EUID and prefer localStorage for both modules. (prebid#9968) Schain module: do not share schain objects across different bid requests (prebid#10021) airgridRtdProvider: use provided tag insertion method `loadExternalScript` (prebid#9901) Increment version to 7.53.0-pre Prebid 7.52.0 release Yahoo ConnectId - fixed tests. (prebid#10033) Core & Multiple modules: activity controls (prebid#9802) Yahoo ConnectId - gpp consent module usage. (prebid#10022) Yahoo bid adapter: User sync pixels, consent signals update (prebid#10028) Core: fix `markWinningBidAsUsed` to set bid as winning and not just rendered (prebid#10014) Adriver Id System: add cfa param to url (prebid#10024) Undertone Bid Adapter : added GPP and video placements (prebid#10016) Oxxion Analytics Adapter : initial adapter release (prebid#9682) Connect id : storage duration updates and no longer require puid or he to be set (prebid#9965) Update ad generation adapter 1.6.0: update userSync (prebid#9984) fix module type (prebid#10019) Stv Bid Adapter: add schain support (prebid#10010) GrowthCode RTD : initial release (prebid#9852) ShareThrough Bid Adapter : fix playerSize (prebid#10011) chore: update default video placement value [PB-1560] (prebid#9948) Smartadserver Bid Adapter: support GPID (prebid#10004) Criteo Adapter: Add support of bcat/badv/bapp (prebid#10013) Zeta Global SSP Bid Adapter: Added support for video.plcmt (prebid#10009) Pair Id System: less noisy errors (prebid#9998) Increment version to 7.52.0-pre Prebid 7.51.0 release Appnexus Bid Adapter: added uol as an alias (prebid#10002) Adquery Bid Adapter : added bid request: version and bidPageUrl (prebid#9946) MinuteMedia Bid Adapter: support sua and plcmt params (prebid#9997) Adkernel Bid Adapter: add didnadisplay alias (prebid#10000) Adrino Bid Adapter: pass adUnitCode to adserver (prebid#9993)
…10005) * remove specific code to pubmatic * remove specific code to smartadserver bidder * update unit tests * remove rubicon specific code * refactor method * refactor this.#setBidderOrtb2 * fix lint issue * add ortb2 user.keywords on appnexs * try fix appnexus case * small refactor inspired on prebid#9952 * try fix appnexus * Revert "try fix appnexus" This reverts commit db1fc08. * Revert "small refactor inspired on prebid#9952" This reverts commit 3959260. * Revert "try fix appnexus case" This reverts commit 919540a. * Revert "add ortb2 user.keywords on appnexs" This reverts commit e2c5747.
move bidder specific handling to ortb2 except appnexus
Type of change
Description of change
bidder specific handling removal : use ortb2 instead
related to