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

PB SDK ORTB issues #1017

Open
bretg opened this issue Jul 1, 2024 · 9 comments
Open

PB SDK ORTB issues #1017

bretg opened this issue Jul 1, 2024 · 9 comments

Comments

@bretg
Copy link
Contributor

bretg commented Jul 1, 2024

Describe the bug

Found a couple of issues while testing first party data (FPD) on PB SDK:

  1. AdUnitConfig.setOrtbConfig() (global-level) is not available in Prebid SDK v2.2.3 despite what the doc says. (we think this probably works for Android but not iOS)
  2. adUnit.setOrtbConfig() is global for iOS rather than imp-level as described in the docs.

Expected behavior

Arbitrary ORTB should work as documented in https://docs.prebid.org/prebid-mobile/pbm-api/ios/pbm-targeting-ios.html#arbitrary-openrtb

@jsligh
Copy link
Collaborator

jsligh commented Jul 8, 2024

@bretg

  1. AdUnit.setOrtbConfig() and AdUnitConfig.setOrtbConfig() are both available on Android. onIOS it needs to either be AdUnit.setOrtbConfig() or AdUnitConfig.ortbConfig = "new json string". I will update the iOS section in the docs with this code.
  2. Thoughts on just removing the impression level from the docs? I believe we had talked about just doing it at the global level as it allows people to customize the impression level things as well.

@bretg
Copy link
Contributor Author

bretg commented Jul 8, 2024

Thoughts on just removing the impression level from the docs? I believe we had talked about just doing it at the global level as it allows people to customize the impression level things as well.

How would they do that and merge to an existing imp object in the array? We tried and it ended up creating a new imp object.

@jsligh
Copy link
Collaborator

jsligh commented Jul 8, 2024

@bretg I guess I didn't realize this would be wanted/needed until now. I thought previously we had just discussed when merging JSON arrays that we would just add to end and remove dupes. I assume that the imp id's are unique? I will create a check that if we are looking at changing an impression, check and see if any of the id's match and then merge those.

@bretg
Copy link
Contributor Author

bretg commented Jul 8, 2024

It's pretty unfriendly to make the caller figure out the imp.id. We need the imp-level ORTB merge.

@jsligh
Copy link
Collaborator

jsligh commented Jul 8, 2024

Is there a unique identifier to be able to tell which imps need to be merged? Or do you want it merged into every imp?

@bretg
Copy link
Contributor Author

bretg commented Jul 8, 2024

The "AdUnit" object translates to an imp object... any method on that object should be able to put the results in the right location.

Let's step back. I'll re-state the original requirements:

  1. we need a way for SDK users to define arbitrary OpenRTB at the global (cross-adunit) level. Example use case: PBS cache commands.
  2. we need a way for SDK users to define arbitrary OpenRTB at the level of a single adunit. Example use case: custom PMP

To those requirements, I'll state a design consistency preference that global ORTB be treated similarly to other global parameters - i.e. that method should be on Targeting.shared.

@YuriyVelichkoPI
Copy link
Contributor

YuriyVelichkoPI commented Jul 9, 2024

Hi @jono, @bretg, sorry, I remember that I promised to take a look at this ticket and provide implementation spec, but unfortunately, I didn't have the bandwidth. I'll do my best to accomplish it this week.

However, as I told you before, it won't be a simple feature because Swift and JAVA, unlike js, don't manage the JSONs in a generic way. I don't support the implementation of merging, like merging "random" JSONs, which are basically random strings. We need to implement the merging of the internal RTB models (the model is already implemented in the SDK).

So the SDK should deserialize arbitrary RTB param into the internal model, merge this model into the model built by SDK, and serialize the result into the JSON (build OpenRTB request) for the request. If SDK controls the structure and rules we will avoid such issues like invalid ids, etc.

If it makes sense, I'll work on the details later this week.

@YuriyVelichkoPI
Copy link
Contributor

YuriyVelichkoPI commented Jul 25, 2024

Hi @bretg, @jsligh,

Investigation

I've been experimenting with the current implementation and we can adapt it if we don't want to work on the internal-model level.

The current method adUnit.setOrtbConfig() merges given values starting from the global level.

For instance, the following code:

adUnit.setOrtbConfig("{\"test_rtb\":1, \"app\" : {\"bundle\": \"org.prebid.PrebidDemoSwift_TEST\"}, \"device\": {} }")

changes the request in the following way:

  • adds test_rtb on the global request level
  • changes the app.bundle value
  • keeps the device as is.

However, if we want to change the impression using the setOrtbConfig: like this:

adUnit.setOrtbConfig("{\"test_rtb\":1, \"app\" : {\"bundle\": \"org.prebid.PrebidDemoSwift_TEST\"}, \"device\": {}, \"imp\": [{\"banner\": {\"api\": [7]}}]}")

The algorithm builds wrong requests. It adds a new object instead of modifying the existing one. And it also breaks the json (❗️). You can try the code snippet to reproduce the error.

In any case, publishers can't use adUnit.setOrtbConfig to modify Imp-level data properly.

Objectives

In order to implement the requirements and keep the current approach (merging of random JSONs), we need to

  • Introduce Targeting.shared.setOrtbConfig(_ ortbObject: String?) method.
    • The ortbObject should be applied on the global request level. Basically, repeat the current implementation.
    • TBD: before merging, the global level ortbObject object should be cleaned from the imp object. To not confuse publishers. If they need to set the imp-level config they should use the respective method.
    • the provided ortbObject should be applied to ALL ad requests for all ad formats.
  • Change the current adUnit.setOrtbConfig(_ ortbObject: String?) method in the following way.
    • Data provided in ortbObject should be merged into the respective imp object.
    • Need to fix the error on merging arrays described earlier in this message.
    • TBD: introduce AdUnit.setGlobalOrtbConfig() to have the ability to customize the bid request on the global level but only for this particular ad unit. If AdUnit is provided with GlobalOrtbConfig SDK should ignore the value provided by Targeting.shared.setOrtbConfig in creating the request for this particular ad unit.
  • Add setOrtbConfig() to rendering API. For now, such classes as BannerView do not have it.
  • Add tons of unit tests :)

@ankit-thanekar007
Copy link
Contributor

Hi @YuriyVelichkoPI , @jsligh Is there an ETA for this change ? This would be really helpful to have.

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