prebid / prebid-mobile-android

Prebid Mobile SDK for Android applications
Apache License 2.0
58 stars 84 forks source link

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

Open AbrahamArmasCordero opened 4 months ago

AbrahamArmasCordero commented 4 months ago

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)
  3. Use any GAM Original integration test Case
  4. 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):

Smartphone (please complete the following information):

Additional context Add any other context about the problem here.

bretg commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

i'll make that work then

AbrahamArmasCordero commented 4 months ago

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

YuriyVelichkoPI commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

No default values in the requests. For sure.

AbrahamArmasCordero commented 4 months ago

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 commented 4 months ago

No worries, thanks for your contribution @AbrahamArmasCordero !