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

Bidder specific imp-level params #2335

Closed
patmmccann opened this issue Aug 2, 2022 · 16 comments
Closed

Bidder specific imp-level params #2335

patmmccann opened this issue Aug 2, 2022 · 16 comments
Labels

Comments

@patmmccann
Copy link

patmmccann commented Aug 2, 2022

We're attempting to set playback method in ortb for a single bidder:

videoCopy.PlaybackMethod == nil {

As of prebid 7 js we can use imp.ext.prebid.bidderconfig to set `

pbjs.requestBids({
ortb2: { ext: { prebid: { bidderConfig: ...
} `

for just the video call so the bidderConfig doesn't need to hang off imp, but it seems like it wouldn't get picked up by the adapters which are only looking for these params on the ad unit? Is that accurate or will ext.prebid.bidderConfig.config.ortb2.imp.video.playbackmethod be considered?

@SyntaxNode
Copy link
Contributor

SyntaxNode commented Aug 5, 2022

but it seems like it wouldn't get picked up by the adapters which are only looking for these params on the ad unit? Is that accurate

Yes, this is accurate for both PBS-Go and PBS-Java. This would be imp[].ATTR support which is not included in the specification.

@bretg was this excluded for a specific reason? .. or rather, not included because there was no use case?

@patmmccann
Copy link
Author

@SyntaxNode @bretg another use case is specifying imp.pmp in bidder config.

@spormeon
Copy link

spormeon commented Aug 7, 2022

how does it matter, if your sending: playbackmethod: [1,3], on the mediaTypes: { video: {
and they can only fullfill [1], they will just ignore [3] and bid on [1]?

@bretg
Copy link
Contributor

bretg commented Aug 31, 2022

PBS doesn't support bidder-specific imp-level config. i.e. ext.prebid.bidderConfig.config.ortb2.imp.ANYTHING isn't supported because we don't know WHICH imp to apply it to. Might not be correct to apply it to every imp.

If we need to go down that road, we would need to implement bidderconfig at the imp level. e.g.

{
  imp: {
    ext: {
      prebid: {
        bidderconfig: {
          bidders: ["bidderA"],
          config: {
            ortb2Imp: {
              video: {
                 playbackmethod: 1
              }
            }
          }
        }
      }
    }
  }
}

I would strongly prefer to avoid this level of complexity. The feature is already too complicated.

Seems to me this is much more easily solved in the 33across bid adapter.

@patmmccann
Copy link
Author

Specifying imp.pmp in bidder config seems to be a real and likely popular use case.

@bretg
Copy link
Contributor

bretg commented Sep 1, 2022

imp.pmp

I haven't heard anyone clamoring for this. I'm going to suggest we defer bidder-specific imp-level params until such a time as there's known and somewhat urgent demand. The FPD code is already amongst the most complex part of the whole server.

@bretg bretg changed the title Bidder specific video param Bidder specific imp-level params Sep 1, 2022
@patmmccann
Copy link
Author

patmmccann commented Sep 5, 2022

This proposal is a simplification. We now only allow certain and arbitrary fields to be in bidder config; expanding the list seems conceptually less complicated than keeping it the same.

Also there is specific demand, that's why i posted the issue.

@bretg
Copy link
Contributor

bretg commented Sep 8, 2022

We now only allow certain and arbitrary fields to be in bidder config; expanding the list seems conceptually less complicated than keeping it the same.

I'm fine expanding PBS ext.prebid.bidderconfig to support any global ORTB param.
But global bidderconfig won't work for imps because it's not safe to assume that it should be applied to all imps. We will have to define and develop imp[n].ext.prebid.bidderconfig.

@bretg
Copy link
Contributor

bretg commented Sep 15, 2022

Spoke with @dgirardi on this. We worked out a proposal where bidder-specific imp-level FPD is carried on imp[].ext.prebid.imp.bidderX. As PBS-core is prepping the request for each bidder it should:

  1. scan the imp[].ext.prebid.imp object for that biddercode. If it exists, deep merge the biddercode child object to the imp level, with the bidder-specific values taking precedence. Arrays are overwritten, not merged.
  2. in either case, remove the imp[].ext.prebid.imp object before sending to the bid adapter
  3. ignore unknown biddercodes in imp[].ext.prebid.imp.UNKNOWNBIDDERCODE
  4. no validation
  5. no metrics

e.g.

  • imp[n].ext.prebid.imp.bidderA.pmp.deals[].id:"A"
  • imp[n].ext.prebid.imp.bidderB.pmp.deals[].id:"B"

@SyntaxNode
Copy link
Contributor

SyntaxNode commented Sep 15, 2022

The latest proposal (right above this comment) is no longer imp-level params because you're not targeting a specific impression. You are targeting the bidder params within the impression. If multiple impressions share the same bidder then the same fields would be set, extending the example...

  • imp[0].ext.prebid.imp.bidderA.pmp.deals[].id:"A"
  • imp[1].ext.prebid.imp.bidderB.pmp.deals[].id:"B"
  • imp[2].ext.prebid.imp.bidderA.pmp.deals[].id:"A"

Which turns this into bidder level parameters that we've already implemented in #1042. The only difference is that feature gives precedence to imp data instead of the overrides.

For this to be imp level, we would need to select an ordinal impression or match on the impression id.

@bretg
Copy link
Contributor

bretg commented Sep 23, 2022

I view request-level bidder params as different from this request... it's basically a macro that applies the same value across all imps.

That's not what I meant by imp[n]. Here's a bigger example that might be more clear:

{
  imp: [{
    ext: {
      prebid: {
        storedrequest: { id: "sr1" },  // for the first imp, pull bidders out of a storedrequest
        imp: {
          bidderA: {
            pmp: {
              deals: [{ id:"dealA" }]      // apply this deal to bidderA
            }
          }
        }
      }
  },{
    ext: {
      prebid: {
        storedrequest: { id: "sr2" }, // for the second imp, pull bidders out of a different storedrequest
        imp: {
          bidderB: {
            pmp: {
              deals: [{ id:"dealB" }]     // apply this deal to bidderB
            }
          }
        }
      }
  }]
}

@bretg
Copy link
Contributor

bretg commented Oct 14, 2022

Dug up the slack thread that changed the proposed syntax. Basically it was two-fold:

  1. placing the config right in the adunit is deemed easier for the pubs rather than a separate setBidderConfig that links to an adunit code
  2. Since it's not part of setBidderConfig, we're not limited to using imp[n].ext.prebid.bidderconfig.{bidders,config.ortb2imp}. It seems like a syntax simplification to use imp[n].ext.prebid.imp.BIDDER

However, I do see that the inconsistency in syntax might make the PBS FPD processing feel more difficult. But not sure it would be -- there will need to be a whole separate branch of the code that handles the logic, whether the syntax is imp[n].ext.prebid.bidderconfig or imp[n].ext.prebid.imp

@bretg
Copy link
Contributor

bretg commented Jan 26, 2023

Confirming that this is the syntax that we settled on:

{
  imp: [{
    ext: {
      prebid: {
        storedrequest: { id: "sr1" },  // for the first imp, pull bidders out of a storedrequest
        imp: {
          bidderA: {
            pmp: {
              deals: [{ id:"dealA" }]      // apply this deal to bidderA
            }
          }
        }
      }
  },{
    ext: {
      prebid: {
        storedrequest: { id: "sr2" }, // for the second imp, pull bidders out of a different storedrequest
        imp: {
          bidderB: {
            pmp: {
              deals: [{ id:"dealB" }]     // apply this deal to bidderB
            }
          }
        }
      }
  }]
}

When Prebid Server sees imp[].ext.prebid.imp.BIDDER, the behavior is to:

  1. when passing this imp to that bidder, merge the contents of BIDDER into the imp
  2. remove the imp[].ext.prebid.imp object
  3. leave any imp[].ext.prebid.storedrequest object

@bsardo
Copy link
Collaborator

bsardo commented Jun 15, 2024

I'd like to note that PBS-Go revalidates the imp portion of a bidder request after imp first party data has been merged. The validation rules match what's performed on the incoming request except that bidder param and native validation are skipped. We don't believe it makes sense to specify bidder params or an alternate native string as imp FPD. On top of that, validating these fields is performance intensive.

@bretg
Copy link
Contributor

bretg commented Jun 21, 2024

As the PBS-Java team is implementing this, found that a number of additional clarifications that are required. @bsardo - please confirm how these were implemented in PBS-Go. My apologies for not realizing the complexity of this feature.

  1. imp.ext.prebid.imp.BIDDER must support aliases
  2. imp.ext.prebid.imp.BIDDER must be case insensitive
  3. If imp.ext.prebid.imp.BIDDER does not resolve to an actual bidder or alias in a case-insensitive way, it should be ignored with a warning when in debug mode.
  4. No validation should be done on imp.ext.prebid.imp.BIDDER.{ext, native}
  5. Remove the imp.ext.prebid.imp.BIDDER.ext.prebid.bidder object if it's specified. That's way too circular and dangerous.
  6. The contents of imp.ext.prebid.imp.BIDDER take precedence in the merge if the field already exists in the request.
  7. The resulting imp object should be valid OpenRTB per the system schema.

This whole thing is ridiculously complicated at this point -- so much that it seems highly unlikely anyone's actually going to use it. The community should expect strong pushback on any future enhancement requests in this area.

@bretg
Copy link
Contributor

bretg commented Sep 3, 2024

Done with PBS-Java 3.11

@bretg bretg closed this as completed Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

No branches or pull requests

5 participants