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

AirGrid RTD Submodule: Initial Release #7108

Merged
merged 9 commits into from
Aug 2, 2021

Conversation

ydennisy
Copy link
Contributor

@ydennisy ydennisy commented Jun 26, 2021

Type of change

  • Feature

Description of change

This PR adds a new RTD submodule, this module augments the bid Bids/AdUnits for the AppNexus/Xandr bid adapter by attaching segment values to the keywords param.

The flow of the module:

  • Inject a 3rd party script into the DOM, we do not need to wait for this script to load, we do this in an async manner.
  • Fetch audience values from localStorage using the StorageManager
  • If audiences are available augment the bids object

dennis@airgrid.io

Other information

As noted above this module loads 3rd party JS into the page.

@patmmccann
Copy link
Collaborator

Hi, even if other bidders don't currently understand your data, it is important to future proof your adapter now by allowing publishers to write your data to global or the ortb2 object for a bidder that they choose. See the sir data module for an example and the current discussion on #7001

@ydennisy
Copy link
Contributor Author

Hi @patmmccann thanks for taking a look so quick!

I am just fixing up one of the failed tests :)

I appreciate your comment and agree with it - but right now we do not have any relationship with other bidders, meaning we would have no way commercialise this data for ourselves or the publisher.

If your suggestion is a requirement to have this approved - please let me know. Since the segment values are fairly opaque this should not be a deal breaker.

@patmmccann
Copy link
Collaborator

It is a requirement; I think you'll find the discussion on 7001 helpful to see why

@ydennisy
Copy link
Contributor Author

Ok thanks @patmmccann taking a look right now 😄

@ydennisy
Copy link
Contributor Author

Hi @patmmccann , thanks again for taking a look at the PR - I have now read the discussion you linked from #7001

Am I correct in understanding that the following flow would be correct in our use case:

  • we provide a bidders key in the publisher config params, where publishers can decide where to send segments
  • our module then uses setBidderConfig to honour this param

Please let me know if the above implementation would be correct.

Based on the above I have two further questions:

  • Xandr does not support use getConfig in their adapter, so we are ok to proceed with the current implementation in addition to setting the bidder specific config until they switch over?
  • The second question is more about the philosophy of this requirement, I see that you state that RTD modules should not be the ones deciding who to share data with, but rather publishers. If RTD data can only be monetised via a subset of bidders, both in terms of value for the RTD provider and Publisher, why would it make sense to pass these values to all bidders on a page? Also, in some business models I imagine the RTD provider does not charge for the module but rather when their segments are activated, passing their segments to all bidders would make this impossible.

@patmmccann
Copy link
Collaborator

patmmccann commented Jun 27, 2021 via email

@ChrisHuie ChrisHuie changed the title AirGrid RTD Submodule - Initial Release AirGrid RTD Submodule: Initial Release Jun 29, 2021
@ChrisHuie ChrisHuie requested a review from msm0504 July 1, 2021 13:50
@ydennisy
Copy link
Contributor Author

ydennisy commented Jul 1, 2021

Hi @patmmccann I have made the changes as we discussed.

Two questions which have arisen:

  • using setBidderConfig seems to set the config on all bidders, even when a single bidder is provided in the way I have used it, maybe there is an error here.
  • what should be the exact data format / key for AG audiences to be passed via ORTB2? I have seen references to a new segtax format, is this what should be followed?

Would really appreciate you taking another look 😄

EDIT:
There are some failing tests, but these seem to be from other modules.

@ydennisy
Copy link
Contributor Author

ydennisy commented Jul 7, 2021

Hello @patmmccann @msm0504 - just wanted to check in here to see when you would have time to review the PR?

All the best,
Dennis

modules/airgridRtdProvider.js Outdated Show resolved Hide resolved
modules/airgridRtdProvider.js Outdated Show resolved Hide resolved
@ydennisy ydennisy requested a review from msm0504 July 14, 2021 09:06
@patmmccann
Copy link
Collaborator

using setBidderConfig seems to set the config on all bidders, even when a single bidder is provided in the way I have used it, maybe there is an error here.

hmm, take a look at the latest approach at #7001 ; does this suffer from the same issue? Perhaps we have a bug.

what should be the exact data format / key for AG audiences to be passed via ORTB2? I have seen references to a new segtax format, is this what should be followed?

you can put data in site.ext.data.airgrid or you could put it in site.content.data if you want to follow the spec in #6057 with an enumerated segtax; or you could get your own segtax enumeration.

see https://docs.prebid.org/features/firstPartyData.html ; openrtb pull 65 ( InteractiveAdvertisingBureau/openrtb#65 ) and adcom pull 22, linked within the iab pull

@ydennisy
Copy link
Contributor Author

@msm0504 sorry I missed the ping - you are once again correct, this is now rectified in the latest commit.

@patmmccann seems the new segtax format is still a little way off being finalised - I will submit to being added to that list but for now I have kept the data in user.ext.data.airgrid.

Your suggestion was site.ext.data.airgrid, but this feels where contextual / page related data would go, rather than audience / segment information. Let me know if I have misunderstood.

@msm0504
Copy link
Contributor

msm0504 commented Aug 2, 2021

@patmmccann I've approved this PR. Have all of your comments been addressed? Can this be merged?

@patmmccann patmmccann merged commit adb1731 into prebid:master Aug 2, 2021
@patmmccann
Copy link
Collaborator

I merged, thanks @msm0504 !

@ydennisy
Copy link
Contributor Author

ydennisy commented Aug 4, 2021

Excellent thank you @patmmccann @msm0504 :)

@patmmccann
Copy link
Collaborator

prebidtappx pushed a commit to prebidtappx/Prebid.js that referenced this pull request Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants