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

Not setting includewinners and includebidderkey flags to false for the request to PBS #780

Open
AbrahamArmasCordero opened this issue Jul 9, 2024 · 15 comments

Comments

@AbrahamArmasCordero
Copy link
Contributor

Describe the bug

Even though we are not sending the includebidderkeys and sending includewinners=true the testing prebid server is assuming that the flag is true and sending the key-values like we are using a Send All Bids workflow, so for solving this issue and having only the key-values needed for Send Top Bid Workflow is specifying either true or false is necessary.

This will also enable the PBS to implement any default value.

To Reproduce
Steps to reproduce the behavior:

  1. Use the Kotlin Demo
  2. Set the flags before initialize method like so ->
PrebidMobile.setIncludeWinnersFlag(true)
PrebidMobile.setIncludeBidderKeysFlag(false)
  1. Use any GAM Original integration test Case
  2. see error

Expected behavior
if we have includewinners=true and no includebidderkeys is present in the skeleton request sent to PBS we are suppose to get a Send Top Bid key-values but instead we are getting a Send All Bids key values.

Screenshots
With postman i was able to showcase this ->
Im setting up a Post HTTP Call with the url https://prebid-server-test-j.prebid.org/openrtb2/auction and copy pasting the logged request from prebid from the logcat

Request with only includewinners set explicitly to true
image

Response with only includewinners set explicitly to true
image

Request with flags set explicitly to true and false
image

Response with flags set explicitly to true and false (only top bid key-values are received)
image

Desktop (please complete the following information):

  • OS: Windows
  • Browser Chrome
  • Version [e.g. 22]

Smartphone (please complete the following information):

  • Device: Android Pixel 7a
  • OS: Android 13
  • Browser stock browser
  • Version [e.g. 22]

Additional context
Add any other context about the problem here.

@bretg
Copy link
Collaborator

bretg commented Jul 9, 2024

Sorry, but the SDK should not be sending targeting values. This should be managed on the Prebid Server side where it can be changed globally.

  1. Values coming from the SDK are always the highest priority during the merge with stored requests.
  2. Once an app is live in the wild, it takes a long time to get people to upgrade.
  3. The includewinners and includebidderkey flags are adops related - if the app hardcodes them, the AdOps team cannot change their minds if they wanted to change the line item structure.

In short, the app should avoid hardcoding unnecessary fields because it severely ties the hands of those who have to maintain it.

So, @AbrahamArmasCordero , the way I recommend you solve this problem is to work with your Prebid Server provider. The default Prebid Server value for includewinners and includebidderkeys is false. Therefore, your Prebid Server team has created your "top-level stored request" with these values set to true. They just need to modify those values in their database/file to false or to remove them.

If you're using https://prebid-server-test-j.prebid.org/openrtb2/auction, don't worry about it. You can control this in your production Prebid Server.

As a lower priority enhancement, it would be reasonable to publish different test top-level requests in Prebid's test PBS covering different combinations of includewinners and includebidderkeys.

@YuriyVelichkoPI
Copy link
Contributor

Totally agree with @bret! The issue and fix look relevant to SDK.

Moreover, the provided fix will silently break the current behavior.

Also. @AbrahamArmasCordero , please share the code snippets for the requests so we can reproduce them.

@AbrahamArmasCordero
Copy link
Contributor Author

I also agree with @bretg i have read the PBS documentation but the original problem and root of this being in the mobile sdk was that my provider is not able to provide me with this changes for now. See issue #651

I'm discussing with the support of my provider since last year when i opened the issue but we are still working on this.

What i would like to do is to have this included in the next version, and remove it if we agree we should not provide this feature from the SDK.


Sorry i just included screenshots, here are the code snippets of the requests, as said i sent them to the https://prebid-server-test-j.prebid.org/openrtb2/auction with postman HTTP POST. In postman i used the default headers that postman includes and no other changes were made.

Request with only includewinners set explicitly to true ->

{"imp":[{"id":"c52914f5-218f-4962-99c7-299253f9aa53","instl":1,"clickbrowser":0,"banner":{"pos":7,"format":[{"w":411,"h":845}]},"video":{"mimes":["video\/mp4","video\/3gp","video\/hls","video\/webm","video\/mov"],"minduration":5,"maxduration":120,"protocols":[2,5,3,6,7,8],"w":411,"h":845,"minbitrate":300,"maxbitrate":2000,"placement":5,"playbackmethod":[2,6],"delivery":[3],"api":[1,2,3,5,6,7,4]},"ext":{"prebid":{"storedrequest":{"id":"prebid-demo-video-interstitial-320-480-original-api"}}}}],"id":"c52914f5-218f-4962-99c7-299253f9aa53","app":{"name":"Manual Config Demo","bundle":"com.refinery89.autoconfig","domain":"www.xgn.nl","storeurl":"https:\/\/play.google.com\/store\/apps\/details?id=com.refinery.xgnapp","ver":"1.0","publisher":{"id":"0689a263-318d-448b-a3d4-b02e8a709d9d","name":"TestPublisherName"},"ext":{"prebid":{"source":"prebid-mobile","version":"2.2.3"}}},"device":{"ua":"Mozilla\/5.0 (Linux; Android 14; Pixel 7a Build\/AP1A.240505.005; wv) AppleWebKit\/537.36 (KHTML, like Gecko) Version\/4.0 Chrome\/126.0.6478.134 Mobile Safari\/537.36","lmt":0,"devicetype":4,"make":"Google","model":"Pixel 7a","os":"Android","osv":"14","hwv":"Google Pixel 7a","language":"en","ifa":"8b593943-3f96-43e3-9197-10ca1ad1abe1","h":2400,"w":1080,"connectiontype":2,"pxratio":2.625},"regs":{"gpp":"DBABM~CQBRxHAQBRxHAAfHjBENA8EgAPLAAELAAAiQHNNX3By5IE9LeChyKPsgMAxbIdj4xIQRBgfBMiIFyAIAMBQGECASBA2oJCAAABAAIQZBIAFkFBkAAVgBAAgRADHIQAEMBAJKAUAkgCAQaUYIAAADCIAgYQIQAgAGAFAIgBCBBBEIBAAAABgGAAAFIAIAACABA2gAswECAAHoCQEIBAwgAAAKAAICCBAAAACQJEBACQAABACAQgQAQAABAAAAAAAABCAAIAAAAAEAAAkAIAAAAAIBAAAAAAABAAgEAAQADAAAAAAAQBAECCBAAAAAwAgBEAAAAIIAAAAAAAMAAAAAgAA","gpp_sid":[2,5],"ext":{"gdpr":1}},"user":{"ext":{"consent":"CQBRxHAQBRxHAAfHjBENA8EgAPLAAELAAAiQHNNX3By5IE9LeChyKPsgMAxbIdj4xIQRBgfBMiIFyAIAMBQGECASBA2oJCAAABAAIQZBIAFkFBkAAVgBAAgRADHIQAEMBAJKAUAkgCAQaUYIAAADCIAgYQIQAgAGAFAIgBCBBBEIBAAAABgGAAAFIAIAACABA2gAswECAAHoCQEIBAwgAAAKAAICCBAAAACQJEBACQAABACAQgQAQAABAAAAAAAABCAAIAAAAAEAAAkAIAAAAAIBAAAAAAABAAgEAAQADAAAAAAAQBAECCBAAAAAwAgBEAAAAIIAAAAAAAMAAAAAgAA"}},"source":{"tid":"c52914f5-218f-4962-99c7-299253f9aa53"},"ext":{"prebid":{"storedrequest":{"id":"0689a263-318d-448b-a3d4-b02e8a709d9d"},"cache":{"bids":{},"vastxml":{}},"targeting":{"includeformat":"true","includewinners":"true"}}},"test":1}

Request with flags set explicitly to true and false ->

{"imp":[{"id":"c52914f5-218f-4962-99c7-299253f9aa53","instl":1,"clickbrowser":0,"banner":{"pos":7,"format":[{"w":411,"h":845}]},"video":{"mimes":["video\/mp4","video\/3gp","video\/hls","video\/webm","video\/mov"],"minduration":5,"maxduration":120,"protocols":[2,5,3,6,7,8],"w":411,"h":845,"minbitrate":300,"maxbitrate":2000,"placement":5,"playbackmethod":[2,6],"delivery":[3],"api":[1,2,3,5,6,7,4]},"ext":{"prebid":{"storedrequest":{"id":"prebid-demo-video-interstitial-320-480-original-api"}}}}],"id":"c52914f5-218f-4962-99c7-299253f9aa53","app":{"name":"Manual Config Demo","bundle":"com.refinery89.autoconfig","domain":"www.xgn.nl","storeurl":"https:\/\/play.google.com\/store\/apps\/details?id=com.refinery.xgnapp","ver":"1.0","publisher":{"id":"0689a263-318d-448b-a3d4-b02e8a709d9d","name":"TestPublisherName"},"ext":{"prebid":{"source":"prebid-mobile","version":"2.2.3"}}},"device":{"ua":"Mozilla\/5.0 (Linux; Android 14; Pixel 7a Build\/AP1A.240505.005; wv) AppleWebKit\/537.36 (KHTML, like Gecko) Version\/4.0 Chrome\/126.0.6478.134 Mobile Safari\/537.36","lmt":0,"devicetype":4,"make":"Google","model":"Pixel 7a","os":"Android","osv":"14","hwv":"Google Pixel 7a","language":"en","ifa":"8b593943-3f96-43e3-9197-10ca1ad1abe1","h":2400,"w":1080,"connectiontype":2,"pxratio":2.625},"regs":{"gpp":"DBABM~CQBRxHAQBRxHAAfHjBENA8EgAPLAAELAAAiQHNNX3By5IE9LeChyKPsgMAxbIdj4xIQRBgfBMiIFyAIAMBQGECASBA2oJCAAABAAIQZBIAFkFBkAAVgBAAgRADHIQAEMBAJKAUAkgCAQaUYIAAADCIAgYQIQAgAGAFAIgBCBBBEIBAAAABgGAAAFIAIAACABA2gAswECAAHoCQEIBAwgAAAKAAICCBAAAACQJEBACQAABACAQgQAQAABAAAAAAAABCAAIAAAAAEAAAkAIAAAAAIBAAAAAAABAAgEAAQADAAAAAAAQBAECCBAAAAAwAgBEAAAAIIAAAAAAAMAAAAAgAA","gpp_sid":[2,5],"ext":{"gdpr":1}},"user":{"ext":{"consent":"CQBRxHAQBRxHAAfHjBENA8EgAPLAAELAAAiQHNNX3By5IE9LeChyKPsgMAxbIdj4xIQRBgfBMiIFyAIAMBQGECASBA2oJCAAABAAIQZBIAFkFBkAAVgBAAgRADHIQAEMBAJKAUAkgCAQaUYIAAADCIAgYQIQAgAGAFAIgBCBBBEIBAAAABgGAAAFIAIAACABA2gAswECAAHoCQEIBAwgAAAKAAICCBAAAACQJEBACQAABACAQgQAQAABAAAAAAAABCAAIAAAAAEAAAkAIAAAAAIBAAAAAAABAAgEAAQADAAAAAAAQBAECCBAAAAAwAgBEAAAAIIAAAAAAAMAAAAAgAA"}},"source":{"tid":"c52914f5-218f-4962-99c7-299253f9aa53"},"ext":{"prebid":{"storedrequest":{"id":"0689a263-318d-448b-a3d4-b02e8a709d9d"},"cache":{"bids":{},"vastxml":{}},"targeting":{"includeformat":"true","includewinners":"true", "includebidderkeys":"false"}}},"test":1}

@YuriyVelichkoPI
Copy link
Contributor

@AbrahamArmasCordero, thanks for the details and the requests.

If it's necessary to send these keys from the SDK the fix should be a bit different.

If the publisher didn't provide any values for these properties, they shouldn't appear in the request at all. Only if publisher used setIncludeWinnersFlag or setIncludeBidderKeysFlag the respective values should appear in the request.

@AbrahamArmasCordero
Copy link
Contributor Author

i'll make that work then

@AbrahamArmasCordero
Copy link
Contributor Author

@YuriyVelichkoPI done both PR's hope this works, if anything happens tell me.

@YuriyVelichkoPI
Copy link
Contributor

@YuriyVelichkoPI done both PR's hope this works, if anything happens tell me.

Haven't reviewed it precisely yet, but please add Unit Tests. They will help not to break the behaviour in the future.

@bretg
Copy link
Collaborator

bretg commented Jul 10, 2024

Could this problem be solved with arbitrary openrtb rather than creating a temporary set of functions?

https://docs.prebid.org/prebid-mobile/pbm-api/ios/pbm-targeting-ios.html#arbitrary-openrtb

adUnitConfig.setOrtbConfig("{\"ext\":{\"prebid\":{\"targeting\":{\"includewinners\":false, \"includebidderkeys\":false}}}")

Though honestly, it appears that the arbitrary openrtb feature may not be fully baked. Perhaps Android works but not sure about iOS.

@YuriyVelichkoPI
Copy link
Contributor

@bretg functions are not new. But they work incorrectly right now.

And yes, once the arbitrary openrtb feature works it should be used in most cases.

@AbrahamArmasCordero
Copy link
Contributor Author

Update: After talking with support they are not going to make available this flags for modification because they are sending Send All Bids + Sent Top Bid key-values every time so it's a decision we can make when doing the configuration of the GAM line items. So if we add this because i said we plan to remove when appnexus fixes this issue, it's not an issue for them so we are not removing this never if we leave it

So canceling this PR and if you think is appropriate i can remove all the code related to this flags, i think so since as you said we have arbitrary openrtb feature

@YuriyVelichkoPI
Copy link
Contributor

YuriyVelichkoPI commented Jul 12, 2024

As for me the PRs make sense because they fix behaviour of these properties. Right now, properties work incorrectly an publisher can't send a false value.

So, adding tests will be a good final point for them before merging.

@bretg
Copy link
Collaborator

bretg commented Jul 12, 2024

Please don't default includewinners or includebidderkeys to anything. If the SDK wants to override what's in the StoredRequest, that's fine, but it could be awkward for AdOps teams if a value is always sent.

@YuriyVelichkoPI
Copy link
Contributor

No default values in the requests. For sure.

@AbrahamArmasCordero
Copy link
Contributor Author

As for me the PRs make sense because they fix behaviour of these properties. Right now, properties work incorrectly an publisher can't send a false value.

So, adding tests will be a good final point for them before merging.

i'll try to finish the PR then this week, sorry for the waiting.

@YuriyVelichkoPI
Copy link
Contributor

No worries, thanks for your contribution @AbrahamArmasCordero !

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

No branches or pull requests

4 participants