-
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
Mobian Bid Adapter : push context data to GAM #12389
Conversation
73cf44b
to
0316c37
Compare
@ChrisHuie I saw this PR didn't make it into the release this week, are there any updates we need to make on our end? |
modules/mobianRtdProvider.js
Outdated
@@ -70,6 +71,18 @@ function getBidRequestData(bidReqConfig, callback, config) { | |||
deepSetValue(ortb2Site, 'ext.data.mobianGenres', genres); | |||
deepSetValue(ortb2Site, 'ext.data.apValues', apValues); | |||
|
|||
setKeyValue('mobianRisk', mobianRisk); |
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.
RTD modules can provide targeting via getTargetingData
- https://docs.prebid.org/dev-docs/add-rtd-submodule.html#gettargetingdata
Would that work for your use case? I think doing it like this has issues - it doesn't necessarily happen at the right time, so you may be setting up targeting for the wrong slot or it may be cleared later by prebid's targeting 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.
So what we need to set the targeting for every ad slot / unit using the googletag API as shown in my code.
How would you approach this using getTargetingData
? You mean invoking setKeyValue
from within the getTargetingData
function?
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.
No - getTargetingData
returns a map of maps (ad unit code -> key -> value), which is taken into account by prebid targeting APIs (getAdserverTargeting
, setTargetingForGPTAsync
& co), which in turn call the GPT targeting APIs.
It's a way to integrate your targeting with Prebid - compared to what you have here, it would only be applied when and where the publisher asks for targeting, and they'd know how to configure it.
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 do not need to integrate into the bid, we just need to set the targeting on a page level (https://developers.google.com/publisher-tag/guides/key-value-targeting#set_targeting)
Browsi is already doing this:
https://github.com/prebid/Prebid.js/blob/master/modules/browsiRtdProvider.js#L70
Are they doing it 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.
we decided to greenlight your implementation of this function, however, you should modify the browsi adapter as part of your pr to import the function and use the duplicated code you identified
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.
Thank you. I made the Browsi update but it breaks their tests. I pushed the update for you to see.
modules/mobianRtdProvider.js
Outdated
setKeyValue('ap_a0', apValues.a0 || []); | ||
setKeyValue('ap_a1', apValues.a1 || []); | ||
setKeyValue('ap_p0', apValues.p0 || []); | ||
setKeyValue('ap_p1', apValues.p1 || []); |
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.
this is a lot of keys and could be consolidated to one, eg mobian=genre:1,emotion=2, etc
Also you should make the key names configurable, and each one should be behind config if it is to be set at all
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 believe the single value solution works for our clients to be able to properly target from GAM, which is what we need. Likewise, the key names should not be configurable but predefined, for the same purpose.
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 disagree, you can target from gam using my suggestion and I am not sure why configurable keynames might break gam either?
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.
Could you indicate how you would target from gam using your suggestion, and also what you mean with configurable keynames specifically? Can you provide examples?
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.
On configuration - the first parameter passed to init
is configuration as set by the publisher. You could accept a prefix
parameter that controls the key names and/or turns them off. for example
pbjs.setConfig({
realTimeData: {
dataProviders: [{
name: 'mobianBrandSafety',
params: {
prefix: 'custom'
}
}]
}
})
would give you {name: 'mobianBrandSafety', params: { prefix: 'custom' }}
as the first argument to init
; and you could use that to set e.g. customSentiment
instead of mobianSentiment
. Setting null
or false
could turn it off.
More parameters could be used to turn off / customize only some keys, but how to do that exactly depends on what makes sense for your use case. You could have one for each ( sentiment: 'customSentiment', emotion: 'customEmotion'
, etc), or it may make more sense to separate them into clusters. Without understanding exactly what you're doing, I'd expect the ap_
keys to be different from the rest for example.
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.
Updated code to enable configuration, and updated tests accordingly. Looking forward to your comments.
Type of change
Bugfix
Feature
New bidder adapter
Updated bidder adapter
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Does this change affect user-facing APIs or examples documented on http://prebid.org?
Other
Description of change
gptUtils.js