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

How to create new RTD submodule #2268

Merged
merged 9 commits into from
Oct 21, 2020
Merged

How to create new RTD submodule #2268

merged 9 commits into from
Oct 21, 2020

Conversation

omerDotan
Copy link
Contributor

Not sure about how we want it organized on the menu, so step 6 will need some update

@bretg
Copy link
Contributor

bretg commented Aug 24, 2020

Cool - thanks. Will do a pass at editing today.

@bretg
Copy link
Contributor

bretg commented Aug 24, 2020

@omerBrowsi, @Asafsham - did a round of edits, but turns out I don't understand the getData() function.

It's not on the diagram, but from the code it looks like what RTD-core does is:

  1. Call getData() in setTargetsAfterRequestBids. It expects data in a certain format and assigns these values to targeting.
  2. Call getData() in requestBidsHook. It expects data in a different unknown format. Don't know what it does with these.

My issues with this design:

  • the getData() function doesn't know whether its being called for targeting or for bidrequest data. How is it supposed to know what format to create?
  • I don't like that the same function creates two entirely different sets of data.
  • There appear to be two ways of modifying bidRequests: getData() and updateBidRequest()?

Let's discuss this in the Tools meeting on Tues. If I understand this correctly, I'd like to propose the following changes:

  1. document that the way to inject bidRequest data is with the updateBidRequest hook.
  2. change getData() to getTargetingData() and make it optional like all the others. And add it to the diagram.

Copy link
Contributor

@bretg bretg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getData isn't documented properly yet -- open questions in the main thread.

@omerDotan
Copy link
Contributor Author

@bretg the getData() appears in the diagram as addTargeting (this is a mistake)
It can be used to set data to GPT only or both GPT and bidders (depends on auction delay parameter)
In both cases, we need it to return data in the same format

I agree this can be optional (we might want to make the init function optional as well to make it as light as possible)

@bretg
Copy link
Contributor

bretg commented Aug 25, 2020

@omerBrowsi - we covered this in the Tools meeting today. The suggestion we came up with is to update the code before any other sub-modules are built:

  • change getData() to addTargeting()
  • call addTargeting() only from setTargetsAfterRequestBids. Remove it from requestBidsHook

This makes the diagram correct and simplifies how modules are supposed to update the bid request: they use updateBidRequest()

@omerDotan
Copy link
Contributor Author

@bretg
Thanks, I'll commit the changes soon.
Keep in mind it makes the auctionDealy param useless - meaning we will never delay the auction.
If the provider (submodule) will want to update the bidRequest with data that is not available immediately (e.g AJAX), he will have trouble doing it.

@omerDotan
Copy link
Contributor Author

changes are on the pull request -
prebid/Prebid.js#5651

@bretg
Copy link
Contributor

bretg commented Aug 28, 2020

Keep in mind it makes the auctionDealy param useless - meaning we will never delay the auction.
If the provider (submodule) will want to update the bidRequest with data that is not available immediately (e.g AJAX), he will have trouble doing it.

Sorry, but I don't understand this. Of course RTD modules need the option of delaying the auction. Are you saying that somehow "getData()" supported a delay but updateBidRequest() doesn't?

If that's the case, then we need to (quickly) re-think the interface -- it's not useful without the ability to delay. What are the options? I'm out of my league, but the obvious approach would seem to be replacing updateBidRequest and auctionInit functions similar to addTargeting().

The requirement is that the sub-module be able to

  • delay the auction for the allowed time
  • inspect and update the bidRequest
  • inspect and update the auction

@omerDotan
Copy link
Contributor Author

the getData function is responsible for retrieving the data from providers (submodules)
it works with callback, that way we make it async (allowing us to delay the process)
In order to delay properly, we have to use hooks (because hooks wait for a function call to proceed, while events are not)

The flow was -

  • use setTargetsAfterRequestBids in case auction delay wasn't set = the auction flow is as usual but after bid request, we hold the process and invoking getData to place key values on GPT

  • use requestBidsHook if auction delay is set = hold the process before the auction and place the data retrieved from providers (using getData) to both GPT and bidders

Now -
since we removed the gedData functionality from requestBidsHook it's not possible (in an easy way) to delay the auction (the delay is only on setTargetsAfterRequestBids).
It's possible to modify the bis request using the updateBidRequest event - but this is a sync code so it's not suitable for cases of retrieving data using AJAX.

I suggest we keep the same logic as before, meaning using a different hook in case the auction delay is set or not.
The provider will be able to change the bid request by using the updateBidRequest and/or by setting auction delay.
If we want the provider to be able to send different data for GPT and bidders (or not send to one of them) we can add another function (similar to getData) or change the expected response of the getData function so it will return separate data for GPT and bidders.

Hope I managed to explain my meaning, let me know if you have any questions

@bretg
Copy link
Contributor

bretg commented Aug 28, 2020

I don't understand the technical reality here - why we can't delay the call to the sub-module getBidRequestData(). But this level of javascript async is beyond where I want to go.

We can revert to a getData()-type approach if you can explain two things to me:

  1. how does a sub-module know whether it's supposed to return targeting data or bidrequest data? It doesn't seem impossible for a single sub-module to want to return both types of data.
  2. what would bidrequest return data look like and how is it merged into the actual bidrequest? Particularly if the desired affect is to remove bidders. The documentation gave an example of what targeting data looked like but didn't explain the bidRequest response.

I'm groping in the dark here, but here's a cut at a path-forward:

  1. change "addTargeting()" to "getTargetingData()". When a sub-module has this function called, it knows specifically it's returning targeting data.
  2. add a new "getBidRequestData()" function that's called from the requestBidsHook. This function is given the array of AdUnits in the auction which it can edit however it sees fit. It's basically the same thing as the current updateBidRequest but works with the delay. If we have to document that there are two ways of editing adunits, so be it.

Here are examples of the kind of data someone might want to inject into the auction:

{
   "*": {.  // all adunits
     "fpd":
       "context": {
         "pagecat": ["IAB1-4", "IAB-9-1"],
         "localweather": "rainy"
       }
     }
   },
   "top_adunit": {
     "fpd": { "context": { "viewability": 1 }},
   },
   "nav_adunit": {
     "fpd": { "context": { "viewability": 0.7 }},
   },
   "bottom_adunit: {
     "fpd": { "context": { "viewability": 4 }},
   }
}

And here are some examples of other edits they might want to make

  • remove bidderJ from all AdUnits because this is the 5th page refresh and bidderJ probably won't bid anymore
  • remove bidderK from all AdUnits because this request is from countryX and bidderK probably won't bid

I want the documentation to list these examples and outline how a sub-module would implement the edits.

@snapwich
Copy link
Contributor

To delay the auction you do need to use a hook (specifically the requestBids hook), but there's no reason the RTD module needs to use the same submodule API for both the requestBids and bidsBackCallback. I agree with @bretg that it could be pretty confusing.

For instance, what if a bidder wants to just delay one or the other? If the sub-module implements getData it will be called, and possibly delay, both requestBids and bidsBackCallback.

@bretg bretg merged commit 9a625db into prebid:master Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants