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

Rename indexExchange to ix (#657) #712

Merged
merged 1 commit into from
Oct 22, 2018

Conversation

ix-certification
Copy link
Contributor

  • Fixes the issues addressed in IndexExchange vs ix #657.
  • Rename package from indexExchange to ix
  • Fixes siteId being incorrect type, Prebid.js sends as a string.
  • We do not currently support video for Prebid Server.
  • Added default endpoint for ix.
  • Response will match ad unit code based on bid id.

@dbemiller
Copy link
Contributor

dbemiller commented Oct 10, 2018

@ix-certification This breaks many things :/.

Prebid Mobile and AMP use Stored Requests. Every PBS host company has their own data, and all that data will still use indexExchange. If you just delete it, suddenly indexExchange will be an unrecognized Bidder, and everyone's pages will break.

We could deprecate it, and delete it in the distant future (after everyone host PBS has migrated their DB data)... but I don't think it's fair to delete it suddenly and with no window for migration.

@bretg
Copy link
Contributor

bretg commented Oct 11, 2018

I agree that a transition phase would seem appropriate here -- run for a time with both the legacy indexExchange adapter and the improved ix adapter.

@ix-certification - would you happen to have reports on how many requests come from the various Prebid Server clusters?

@dbemiller
Copy link
Contributor

One thing @mkendall07 did discover this morning: according to our metrics, indexExchange returns no valid bids at all, to anyone.

So another option is that we could relax the input validation. Today, PBS returns a 4xx if you use an "unknown" bidder. It was designed that way to catch dumb typos or missing aliases.... but if we're ok losing that, we could run the other bidders and return a req.ext error describing the wrong bidder instead. That change would be backwards compatible, and once we made them, these changes wouldn't really "break" anything, since indexExchange isn't bidding today anyway.

@hhhjort
Copy link
Collaborator

hhhjort commented Oct 11, 2018

After some offline discussion, it seems dbemiller's last idea is the best. With a slight modification, we will relax the constraint to not 4xx any "deprecated" bidders, which will include the old name for the index exchange bidder, while leaving the general case intact (truely unknown bidders will still cause a 4xx). Once that PR goes in, we can merge this if no other issues come up in review.

@hhhjort
Copy link
Collaborator

hhhjort commented Oct 12, 2018

I was going through the legacy request validation logic to determine if the old name would be rejected there after this change. I found some index specific logic in pbs/pbsrequest.go...

			// index requires a different request for each ad unit
			if b.BidderCode != "indexExchange" {
				for _, pb := range pbsReq.Bidders {

and another mention further down. What this bit does is configure each ad unit in a request to refer to a different instance of the indexExchange bidder, so for a multi-adunit request the index bidder would be called once for each ad unit, rather than just being called once and given all the ad units.

Is this still an IndexExchange limitation? The openrtb2 converter that adapts the legacy bidder code to the openrtb2 spec would not do this split for index exchange, so there would need to be additional work to get this to work with openrtb2 if this limitation is still in place.

@hhhjort
Copy link
Collaborator

hhhjort commented Oct 12, 2018

Submitted PR #716 in support of this PR, so existing requests won't break when the name change happens.

Copy link
Collaborator

@hhhjort hhhjort left a comment

Choose a reason for hiding this comment

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

Offline communication: Index exchange will submit a subsequent PR to fix the multiple imp issue. Since we will go from non-working adapter -> partially working adapter -> working adapter, this is still a step forward, so I will approve.

Copy link
Contributor

@dbemiller dbemiller left a comment

Choose a reason for hiding this comment

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

Two tiny suggestions... but not at all worth delaying the PR for, given the severity of the issue being fixed.

func (a *IndexAdapter) Name() string {
return "indexExchange"
func (a *IxAdapter) Name() string {
return "ix"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should return the constant, openrtb_ext.BidderIx here.

Width: bid.W,
Height: bid.H,
DealId: bid.DealID,
CreativeMediaType: "banner",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use the openrtb_ext.BidTypeBanner constant here.

@dbemiller dbemiller merged commit c47da13 into prebid:master Oct 22, 2018
katsuo5 pushed a commit to flux-dev-team/prebid-server-1 that referenced this pull request Dec 1, 2020
katsuo5 pushed a commit to flux-dev-team/prebid-server-1 that referenced this pull request Dec 2, 2020
katsuo5 pushed a commit to flux-dev-team/prebid-server-1 that referenced this pull request Dec 4, 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