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

Add adUnitCode to the imp.ext.prebid exception list #4077

Open
krdzo opened this issue Nov 26, 2024 · 9 comments
Open

Add adUnitCode to the imp.ext.prebid exception list #4077

krdzo opened this issue Nov 26, 2024 · 9 comments

Comments

@krdzo
Copy link

krdzo commented Nov 26, 2024

Hello.
We are making a new adapter for prebid and would like to use imp[].ext.prebid.adunitcode in our requests.
The problem is that we don't have access to it. For example adunitcode is not passed here inside the MakeRequests method, but there was one when sending the request. So adapters don't have access to prebid.js adUnitCode.
Is this a bug or it should work like this?
If this is the intended behavior is there a way for us to have access to it inside the adapter?

@krdzo
Copy link
Author

krdzo commented Nov 26, 2024

Found the issue when the sanitization of impExtPrebid is is done the adunitcode field is not allowed to be passed to bidders:

var allowedImpExtPrebidFields = map[string]interface{}{
openrtb_ext.IsRewardedInventoryKey: struct{}{},
openrtb_ext.OptionsKey: struct{}{},
}

func createSanitizedImpExt(impExt, impExtPrebid map[string]json.RawMessage) (map[string]json.RawMessage, error) {
sanitizedImpExt := make(map[string]json.RawMessage, 6)
sanitizedImpPrebidExt := make(map[string]json.RawMessage, 2)
// copy allowed imp[].ext.prebid fields
for k := range allowedImpExtPrebidFields {
if v, exists := impExtPrebid[k]; exists {
sanitizedImpPrebidExt[k] = v
}
}

Is this intended or could we add adunitcode here.

@bretg bretg changed the title adUnitCode in MakeRequest funciton Add adUnitCode to the imp.ext.prebid exception list Dec 4, 2024
@bretg bretg moved this from Triage to Ready for Dev in Prebid Server Prioritization Dec 6, 2024
@bretg bretg moved this from Ready for Dev to Clarify Request in Prebid Server Prioritization Dec 10, 2024
@bretg
Copy link
Contributor

bretg commented Dec 10, 2024

The PBS-Java team pointed out that adunitcode is NOT the same as all the other exceptions. The current exceptions are all under imp.ext: ae, all, context, data, general, gpid, prebid, skadn, and tid.

This request is under imp.ext.prebid. We don't currently have an exception list for that path. There aren't currently very many fields in imp.ext.prebid, but the bidders is one of them, and that definitely has to be cleaned up.

So, @krdzo - please explain the use case for imp.ext.prebid.adunitcode when there are other options that don't require awkward Prebid Server code:

  1. For prebid.js, you can always use imp.id, which is almost the same as adunitcode. (One exception is that twin adunits will have a '-N' sequence number appended. e.g. "top-medrect-1" and "top-medrect-2")
  2. imp.ext.gpid is the industry standard global placement ID

Assuming the use case is compelling, here are the fields I'm aware of that are under imp.ext.prebid. If we're going to have to support adunitcode, we need to define a separate list of which fields under 'prebid' bidders are allowed to see.

  • adunitcode - it would be ok for bidders to see this.
  • storedrequest - it would be ok for bidders to see this.
  • storedresponse - Bidders definitely shouldn't see this.
  • bidder - bidders and params. Bidders definitely can't see anything but their own values.
  • imp - bidder-specific imp-level FPD. Bidders definitely can't see this.
  • options. various auction options. Bidders definitely can't see this.
  • passthrough - not clear whether bidders should see this. Assume no for now.

@krdzo
Copy link
Author

krdzo commented Dec 11, 2024

Hello @bretg
I explained it a little bit in our PR for new adapter but in a little bit more detail we are using adunitcode to uniquely identify a ad placement in our system. On prebid.js side it was one of the only way to do this, the rest of the field were often random values or were always changing. So we wanted to copy that behaviour in to our PBS adapter. So yes we are using it as a quasi gpid (gpid wasn't there when we started to do this and even now it isn't set by a majority of publishers)

So for your points:

  1. if imp.id is always a version of adunitcode for prebid.js, that's enough for us. I thought this was the case but wasn't sure because I couldn't find nowhere in the documentation that it says so.
    So yes we can will just use imp.id instead of adunitcode

  2. we didn't think to use gpid because it's not really a standard. From our experience on prebid.js side not everybody sets it and it still just a community extension of the oRTB standard so we didn't want to migrate to it. Maybe we will change this in the future.

So yes it's ok with us to stay the way it is. We will just use imp.id as adUnitCode.
I will leave up to you if you want to close this issue of leave it open until you see what to do with these fields.

P.S. you said that there are no exceptions for imp.ext.prebid but in PBS-Go there are. As you can see in the code the options and the is_rewarded_inventory fields are forwarded to bidders, so I don't know if you want to change this as you said that at least the "options" field should not be visible from bidders.

options. various auction options. Bidders definitely can't see this.

@bretg
Copy link
Contributor

bretg commented Dec 11, 2024

Thanks for the observations @krdzo . Will discuss with the committee in the next meeting.

@SyntaxNode
Copy link
Contributor

SyntaxNode commented Dec 12, 2024

P.S. you said that there are no exceptions for imp.ext.prebid but in PBS-Go there are.

There are exceptions which are documented here, per the "Adapter Sees?" column.

I notice that adunitcode is included there, but the path is imp.ext.prebid.adunitcode instead of imp[].ext.prebid.adunitcode so it may be missed. We'll review at the next committee meeting.

Simple to address, however if your adapter requires the adunitcode this may be the first instance of an adapter strongly coupled to Prebid.js. It's not safe to assume all traffic is sent from Prebid.js. We should discuss this as well.

@krdzo
Copy link
Author

krdzo commented Dec 13, 2024

@SyntaxNode we don't only relay on adunitcode. We have a fallback on imp[].id, trying to push to change to gpid, but when it's Prebid.js then we use adunitcode, if someone doesn't use PBS with prebid.js then we just use imp[].id and because adunitcode and imp[].id are mostly the same thing in PBS I changed it to just use imp[].id

@bretg
Copy link
Contributor

bretg commented Dec 13, 2024

I notice that adunitcode is included there, but the path is imp.ext.prebid.adunitcode instead of imp[].ext.prebid.adunitcode so it may be missed. We'll review at the next committee meeting.

@SyntaxNode - is the missing [] just a doc problem or part of the code as well?

Underneath imp[].ext.prebid, I'd say only adunitcode and storedrequest should be visible to adapters. Should we think of that as an exception list parallel to the imp[].ext list?

@SyntaxNode
Copy link
Contributor

SyntaxNode commented Dec 13, 2024

@SyntaxNode - is the missing [] just a doc problem or part of the code as well?

It's a docs problem, which may also be why it was overlooked when coded.

Should we think of that as an exception list parallel to the imp[].ext list?

Yes. That works for me.

Underneath imp[].ext.prebid, I'd say only adunitcode and storedrequest should be visible to adapters.

I agree those are fine to share with adapters. We currently allow imp[].ext.prebid.is_rewarded_inventory and imp[].ext.prebid.options, the later which is for the Prebid SDK per this issue.

@SyntaxNode
Copy link
Contributor

I don't see any adapters reading from imp[].ext.prebid.options and the only option defined is for core logic. We may be able to remove that from adapters. Hard to tell if adapters are reading that value server side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Clarify Request
Development

No branches or pull requests

3 participants