optable-targeting: Review follow-ups (stored-request leak, plan resolution, api-timeout) - #6
Open
justadreamer wants to merge 7 commits into
Open
justadreamer wants to merge 7 commits into
justadreamer wants to merge 7 commits into
Conversation
An account can have the module hooks in its execution plan without a module config of its own. ConfigResolver passed the null config to the JSON merger, which threw and failed every hook of the module.
The fallback looked for imp.ext.bidder, which is not a format the core knows. Old-style params sit directly under imp.ext and the core moves them to imp.ext.prebid.bidder for every imp that has no bidders there, so the sampler now applies the same rule per imp instead of falling back only when no imp at all has imp.ext.prebid.bidder.
An account config with "enrichment-percentage": null or "bidder-enrichment-percentages": null overrides the defaults of the properties class, and the sampler then failed on unboxing.
… mixed stored imps When the raw hook deferred the call to the processed hook it returned no_action, so user.ext.optable reached every bidder unless the processed hook was in the plan. The raw hook now always cleans and keeps user.ext.optable in the module context for the deferred call. The raw hook also defers whenever the request or any imp references a stored request, since bidders from stored imps were otherwise never sampled. Sampling now runs only once, at the stage that makes the call. The README now lists the processed hook as required.
An exception in the raw or processed auction request hook failed the hook without applying the cleaner, so user.ext.optable reached the bidders. Both hooks now catch it, log it and still return the cleaner update. The call and the bidders to enrich are set together, so the bidder request hook never sees bidders without a call to await.
…pi-timeout CompositeHookExecutionPlan ignored hooks.default-account-execution-plan, only looked at /openrtb2/auction, took the timeout of the first group of the bidder request stage and cached the per-account result forever. It now only tells whether the bidder request hook is configured for the request, combining the host plan with the account plan (or the default account plan) for the request's endpoint, without caching. Whether the raw hook has run comes from the module context. The call awaited by the bidder request hook is bound by the new api-timeout parameter, or by the time remaining for the auction. The raw stage never runs for amp and video requests, so when the bidder request hook is configured the processed hook now starts the call there instead of passing through without one. When the raw hook has run and the bidder request hook is absent, the processed hook awaits the early call and enriches the request, as before.
…gacy setup Without the raw and bidder request hooks the processed hook called the API and enriched every request, whatever enrich-web, enrich-app and the enrichment percentages said. It now skips disabled traffic sources and only calls the API when sampling selects at least one bidder.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up fixes for the optable-targeting review, one commit per issue.
Why / what changed
user.ext.optable. When the raw hook deferred the call, it returnedno_action, so email/phone/zip/vid reached every bidder unless the processed hook was in the plan. The raw hook now always cleans the request and keepsuser.ext.optablein the module context for the deferred call.ConfigResolver. It now falls back to the host config.imp.ext.bidder. They are now read fromimp.ext.<bidder>using core's reserved-field rule, per imp.enrichment-percentageorbidder-enrichment-percentagesthrew an NPE.CompositeHookExecutionPlan:api-timeoutparameter, or by the remaining auction time when it is not set, instead of the first group's timeout.enrich-web/enrich-appand sampling.Testing
OptableStoredRequestFlowTestruns a request through the raw, processed and bidder-request hooks, covering:appthat comes from a stored request