Adding support for aliases #296
Conversation
…No new unit tests yet.
ghost
assigned 
| liveAdapters[i] = a | ||
| i++ | ||
| } | ||
| // Randomize the list of adapters to make the auction more fair | ||
| randomizeList(liveAdapters) | ||
| // Process the request to check for targeting parameters. | ||
| var targData *targetData = nil | ||
| var targData *targetData |
There was a problem hiding this comment.
I know the old and the new lines are functionally the same. Is there any advantage to not explicitly declaring the value of the variable to be the null value?
There was a problem hiding this comment.
not that I know of, no.
I've been trying out visual studio and it complains about those lines... I think cause it runs golint in the background. I really have no preference either way.
|
5 days with no changes requested by the second reviewer... merging this, per the standard process. @cindyhere33 If you get around to it at some point and find changes, please open issues (or submit new PRs) for them. |
| return fmt.Errorf("request.ext.prebid.aliases.%s refers to unknown bidder: %s", thisAlias, coreBidder) | ||
| } | ||
| if thisAlias == coreBidder { | ||
| return fmt.Errorf("request.ext.prebid.aliases.%s defines a no-op alias. Choose a different alias, or remove this entry.", thisAlias) |
There was a problem hiding this comment.
Should this necessarily return an error?
There was a problem hiding this comment.
definitely debatable... I'm not 100% sure.
My thoughts were: "If they sent this, we would ignore it. If we ignore it, then they'll get better performance (smaller I/O) if they don't send it. Let's encourage that."
I sorta doubt anyone will care either way, though.
There was a problem hiding this comment.
It is debatable. I was thinking there is one case that we would probably want to return an error, but this wouldn't catch it. Do we want to flag when someone aliases one core bidder to another? Like appnexus=rubicon? I don't really see a use case to do this, and if someone does it is quite likely that it will just cause confusion. Perhaps we want to check for this case as well?
We could use a method to pass back warnings, so we can flag curious things like this without invalidation the entire bid request for the bidder. Developers could then see the message when they are trying to debug why the auction isn't happening as they expected. But that is probably out of scope for this PR.
There was a problem hiding this comment.
Yeah, the appnexus=rubicon one is a good case. I definitely did consider making that an error... but that would actually make every new Bidder a breaking change. Today, publishers could alias foo=appnexus, and tomorrow, someone could submit a new Bidder with the name foo. It's very unlikely, but.... if this is an error, then that's technically a breaking change.
So... instead I made appnexus=rubicon legal, and defined the API so that aliases take precedence over "core" bidders. There's a section about it in the aliases docs... but maybe it's worth highlighting? With a "NOTE" or "WARNING" or something like that.
If an alias overlaps with a core Bidder's name, then the alias will take precedence. This prevents breaking API changes as new Bidders are added to the project.
There was a problem hiding this comment.
I've been toying with the idea of "warn, but execute the auction anyway" too... but I lean moderately against it.
Devs are more likely to miss or ignore 2xx responses with an error message stuffed into them than they are 4xx's. If the error message is good, it takes like 30 seconds to fix.
In some cases it's obvious what the user "meant" and we can run an appropriate auction, but in other cases it's not. It's like a deer in a thicket; you can walk in easily, but you're likely to get your antlers tangled, and then it'll be impossible to escape. And if they miss the warnings, and the auction doesn't behave exactly like they expect, they'll log issues about it... which we'll then have to debug.
I'd just rather not go down that road.
* Added alias support to the request validation and bid preprocessing. No new unit tests yet. * Fixed some lint errors. * Added aliases as a return value from cleanOpenRTBRequests. * Added alias support to the core auction logic. * Fixed a bug with the timeout computation. * Made sure that the core biddername gets used for operational metrics. * Fixed a unit test which was broken after the changes. * Cleaner, simpler parse/validate method. * Added some tests, and fixed some bugs. * Added one more use case, and tightened the input validation a bit. * Updated the endpoint docs. * Better alias docs. Fixed the header mismatches.
* Added alias support to the request validation and bid preprocessing. No new unit tests yet. * Fixed some lint errors. * Added aliases as a return value from cleanOpenRTBRequests. * Added alias support to the core auction logic. * Fixed a bug with the timeout computation. * Made sure that the core biddername gets used for operational metrics. * Fixed a unit test which was broken after the changes. * Cleaner, simpler parse/validate method. * Added some tests, and fixed some bugs. * Added one more use case, and tightened the input validation a bit. * Updated the endpoint docs. * Better alias docs. Fixed the header mismatches.
This fixes #11.
Adding alias support to the OpenRTB endpoints. For a description of the feature, see the issue + docs. if those aren't clear enough, let me know so that I can make them better for everyone else.