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

Add advertiser ID for Index, OpenX and PubMatic #3322

Closed
wants to merge 3 commits into from

Conversation

dreischer
Copy link
Contributor

Type of change

  • Other

Description of change

Bid responses often contain details about the advertiser and/or DSP. However, some adapters don't expose these fields. This PR updates the adapters for Index, OpenX and PubMatic to include advertiser ID, advertiser domain and DSP ID (where available).

AppNexus and Rubicon responses already include these fields. They come through like this

"rubicon": {
    "advertiserId": 27636,
    "networkId": 2249
  },

and

 "appnexus": {
    "buyerMemberId": 3861
  },

I followed the same structure, adding the fields in an object named after the bidder, e.g.

"ix": {
  "adomain": ["visa.co.uk"]
  "advbrand": "Visa"
  "advbrandid": 1234
}

@jaiminpanchal27
Copy link
Collaborator

@indexexchange @PubMatic-OpenWrap @jimee02 Please review

@stale
Copy link

stale bot commented Dec 11, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 11, 2018
@dreischer
Copy link
Contributor Author

@jaiminpanchal27 @indexexchange @PubMatic-OpenWrap @jimee02 Please let me know if there's anything I can help with or if there are any questions! I'd be really keen to get this merged!

@stale stale bot removed the stale label Dec 14, 2018
@stale
Copy link

stale bot commented Dec 28, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 28, 2018
@dreischer
Copy link
Contributor Author

@jaiminpanchal27 Any chance anyone else could review this?

Thanks!

@stale stale bot removed the stale label Jan 2, 2019
@stale
Copy link

stale bot commented Jan 16, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 16, 2019
@dreischer
Copy link
Contributor Author

Just bumping this @jaiminpanchal27 @indexexchange @PubMatic-OpenWrap @jimee02 Thanks!

@stale stale bot removed the stale label Jan 17, 2019
@jaiminpanchal27
Copy link
Collaborator

@dreischer Please resolve conflict

@stale
Copy link

stale bot commented Feb 6, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Feb 6, 2019
@dreischer
Copy link
Contributor Author

Yes, I'll update!

@stale stale bot removed the stale label Feb 13, 2019
@pm-harshad-mane
Copy link
Contributor

Hello team,
keys used for PubMatic looks good to me.
I refered PubMatic documentation at https://community.pubmatic.com/display/PA/OpenRTB+Bid+Response+Objects+and+Parameters

@pm-harshad-mane
Copy link
Contributor

Hello team,

I have a concern here, all adapters follow same format for bid object apart from the newly intrduced bidder-specific object in bid. If we want to pass information like DSP-Id, Advertiser-Id, AdDomain then can we define standard keys in bid object that every bidder should follow than creating a bidderspecific object in the bid object? Having standard keys will be easier for Analytics adapters to consume this information.

PubMatic Bid object with current approach looks like follwoing,

bid = {
   cpm: 10,
   width: 728,
   height: 90,
   adm: "",
   .... // other keys
   pubmatic: {   // this key name will be different for IX, OpenX
      dspid: "",   // this key name will be different for IX, OpenX
      advid: "",   // this key name will be different for IX, OpenX
      addomain: []   // this key name will be different for IX, OpenX
   }
}

With proposed changes all bidders will have bid object as following:

bid = {
   cpm: 10,
   width: 728,
   height: 90,
   adm: "",
   .... // other keys
   dspid: "",   // this key name will be SAME for all adapters
   advid: "",   // this key name will be SAME for all adapters
   addomain: []   // this key name will be SAME for all adapters
}

@pm-harshad-mane
Copy link
Contributor

Hello @dreischer and @jaiminpanchal27 ,
Could you please help me with the above concern/query?

@pm-harshad-mane
Copy link
Contributor

Hello @jsnellbaker ,
Could you please help me with the above concern/query?

@jsnellbaker
Copy link
Collaborator

@pm-harshad-mane

I'm not sure if we'd be able to replace the bidder specific object entirely; there is the case that a bidder would have something unique that's not really utilized by others but may want to be tracked.

That said, if we were going to add some new base-line variables - we'd need to ensure that there we identified all the properties that would be ideal for most use-cases and using names that are clear/applicable to those use-cases.

If someone was forced to try to re-use a field to mean something else because there wasn't something that really matched, it could cause some confusion/head-ache.

CCing @mkendall07 for his thoughts.

@pm-harshad-mane
Copy link
Contributor

Hello @jsnellbaker ,

Thanks for your feedback.
I agree that bidder-specific objects cannot be totally avoided.
In the above example, we are discussing about putting dspid, advid, addomain into the bid object as these are common entities for all bidders, bidders who do not want to share these details can leave these fields empty. In the code changes, each bidder is trying to pass information related to the same entities in a different manner, which is not a good practice and will not be helpful for an analytics adapter to consume.

What do you think @mike-chowla ?

@dreischer
Copy link
Contributor Author

@pm-harshad-mane I agree that it would be nice to have a consistent structure/naming across all bidders. However, some bidders already include these fields using custom names, e.g. bid.appnexus.buyerMemberId or bid.rubicon.advertiserId. The PR follows the same pattern, so we are consistent with what is already in place. My concern about introducing new fields would be that we would send redundant data for e.g. rubicon.

@jaiminpanchal27 I've resolved the merge conflict, but one of the automated tests failed. Could you please restart it? I think the error is just a timeout and has nothing to do with this PR.

@dreischer
Copy link
Contributor Author

@jaiminpanchal27 Never mind, I committed another small change to this PR which made the checks run again (with no timeouts this time) 🙂

@mike-chowla It would be great if you could please take a look at this PR. Thanks!

@mike-chowla
Copy link
Contributor

The PubMatic changes here are similar to #3619 but each PR uses different variable names

@mike-chowla
Copy link
Contributor

I also noticed that #3630 is Open for similar functionality on OpenX. I like the how those other 2 PRs are putting fields user a consistently named 'meta' object in the bid. I suspect it's being done that way so they can easily be collected for analytics purposes. @kelvin-chappell can you comment?

@kelvin-chappell
Copy link
Contributor

@mike-chowla In the Guardian's fork we are using the format specified in #3115. And in the hope that this becomes a de facto standard I have submitted PRs for the adapters that give us buyer data:

I would recommend following the same format so that it becomes easy to build an analytics adapter that can read buyer data from any bid adapter, as we have done here.

@pm-harshad-mane
Copy link
Contributor

I agree with @kelvin-chappell

@mike-chowla
Copy link
Contributor

Given the similar PRs (#3619, #3630) are already merged. I'm going to close this PR.

@dreischer If the other PRs are missing fields you need, please open a new PR to add them

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

Successfully merging this pull request may close these issues.

6 participants