optable-targeting: Review follow-ups (stored-request leak, plan resolution, api-timeout)

extra/modules/optable-targeting/README.md

@@ -156,19 +156,21 @@ Sample module enablement configuration in JSON and YAML formats:
 
 Previous versions used only the `processed-auction-request` hook (with `auction-response`), which made the API call
 and enriched the whole request synchronously, blocking the auction. To migrate, keep that hook and add the
-`raw-auction-request` and `bidder-request` hooks as shown above. With them in the plan, the processed hook no longer
-blocks; without them, it keeps the legacy synchronous behavior.
+`raw-auction-request` and `bidder-request` hooks as shown above. With the `bidder-request` hook in the plan, the
+processed hook no longer blocks; without it, it keeps the legacy synchronous behavior. In that mode the whole request is
+enriched, so it is enriched for all bidders whenever sampling selects any of them.
 
 ### Timeout considerations
 
-The `bidder-request` hook timeout is the budget for the API call started in the `raw-auction-request` or
-`processed-auction-request` stage. The call runs in parallel with the rest of the auction, so the actual wait at the
-`bidder-request` stage is usually much shorter than the full roundtrip. The `raw-auction-request` and
+The API call is started in the `raw-auction-request` or `processed-auction-request` stage and runs in parallel with the
+rest of the auction, so the actual wait at the `bidder-request` stage is usually much shorter than the full roundtrip.
+The `api-timeout` module parameter limits the call itself (by default, the time remaining for the auction), and the
+`bidder-request` hook timeout limits how long a bidder request waits for it. The `raw-auction-request` and
 `processed-auction-request` hook timeouts only cover validation and sampling and can be kept short.
 
-**Note:** Do not confuse hook timeout value with the module timeout parameter which is optional. The hook timeout value
-would depend on the cloud/region where the PBS instance is hosted and the latency to reach the Optable's servers. This
-will need to be verified experimentally upon deployment.
+**Note:** Do not confuse these with the module `timeout` parameter, an optional hint passed to the Targeting API. The
+`api-timeout` and hook timeout values depend on the cloud/region where the PBS instance is hosted and the latency to
+reach the Optable's servers, and need to be verified experimentally upon deployment.
 
 The timeout value for the `auction-response` can be set to 10 ms - usually it will be sub-millisecond time as there are
 no HTTP calls made in this hook - Optable-specific keywords are cached on earlier stages and retrieved from the module
@@ -196,6 +198,7 @@ would result in this nesting in the JSON configuration:
 | ppid-mapping                   | no       | map     | none          | This specifies PPID source (`user.ext.eids[].source`) to a custom identifier prefix mapping, f.e. `{"example.com" : "c"}`. See the section on ID Mapping below for more detail.                                                                                                                                                                                                                                            |
 | adserver-targeting             | no       | boolean | false         | If set to true - will add the Optable-specific adserver targeting keywords into the PBS response for every `seatbid[].bid[].ext.prebid.targeting`                                                                                                                                                                                                                                                                          |
 | timeout                        | no       | integer | none          | A soft timeout (in ms) sent as a hint to the Targeting API endpoint to limit the request times to Optable's external tokenizer services                                                                                                                                                                                                                                                                                    |
+| api-timeout                    | no       | integer | none          | A hard timeout (in ms) for the Targeting API call awaited by the `bidder-request` hook. When not set, the call is limited by the time remaining for the auction. See Timeout considerations above. |
 | id-prefix-order                | no       | string  | none          | An optional string of comma separated id prefixes that prioritizes and specifies the order in which ids are provided to Targeting API in a query string. F.e. "c,c1,id5" will guarantee that Targeting API will see id=c:...,c1:...,id5:... if these ids are provided. id-prefixes not mentioned in this list will be added in arbitrary order after the priority prefix ids. This affects Targeting API processing logic  |
 | hid-prefixes                   | no       | string  | none          | An optional string of comma separated id prefixes that should additionally be sent to the Targeting API as resolver hints in `hid=prefix:value` query parameters. See the section on Resolver Hints (hid) below for more detail.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                    |
 | enrichment-percentage          | no       | integer | 100           | Default percentage (0-100) of bid requests per bidder that will receive enrichment data. Set to 100 to enrich all requests, 0 to disable enrichment by default.                                                                                                                                                                                                                                                            |

extra/modules/optable-targeting/src/main/java/org/prebid/server/hooks/modules/optable/targeting/config/OptableTargetingConfig.java

@@ -119,7 +119,8 @@ OptableTargetingModule optableTargetingModule(ConfigResolver configResolver,
                         logSamplingRate),
                 new OptableTargetingProcessedAuctionRequestHook(
                         configResolver,
-                        earlyOptableCallResolver),
+                        earlyOptableCallResolver,
+                        logSamplingRate),
                 new OptableBidderRequestHook(),
                 new OptableTargetingAuctionResponseHook(
                         configResolver,
@@ -136,17 +137,22 @@ BidderEnrichmentSampler bidderEnrichmentSampler(BidderCatalog bidderCatalog) {
     OptableTargetingFlowResolver earlyOptableCallResolver(
             BidderEnrichmentSampler bidderEnrichmentSampler,
             TargetingRequestExecutor targetingRequestExecutor,
-            @Value("${hooks.host-execution-plan:}")
-            String executionPlan,
+            @Value("${hooks.host-execution-plan:}") String hostExecutionPlan,
+            @Value("${hooks.default-account-execution-plan:}") String defaultAccountExecutionPlan,
             JacksonMapper mapper,
             @Value("${logging.sampling-rate:0.01}") double logSamplingRate) {
 
         final CompositeHookExecutionPlan hooksExecutionPlan = CompositeHookExecutionPlan.of(
-                StringUtils.isNoneEmpty(executionPlan)
-                        ? mapper.decodeValue(executionPlan, ExecutionPlan.class)
-                        : null);
+                parseExecutionPlan(hostExecutionPlan, mapper),
+                parseExecutionPlan(defaultAccountExecutionPlan, mapper));
 
         return new OptableTargetingFlowResolver(
                 bidderEnrichmentSampler, targetingRequestExecutor, hooksExecutionPlan, logSamplingRate);
     }
+
+    private static ExecutionPlan parseExecutionPlan(String executionPlan, JacksonMapper mapper) {
+        return StringUtils.isNotBlank(executionPlan)
+                ? mapper.decodeValue(executionPlan, ExecutionPlan.class)
+                : ExecutionPlan.empty();
+    }
 }

extra/modules/optable-targeting/src/main/java/org/prebid/server/hooks/modules/optable/targeting/model/ModuleContext.java

@@ -1,5 +1,6 @@
 package org.prebid.server.hooks.modules.optable.targeting.model;
 
+import com.fasterxml.jackson.databind.JsonNode;
 import io.vertx.core.Future;
 import lombok.Data;
 import org.prebid.server.hooks.modules.optable.targeting.model.config.OptableTargetingProperties;
@@ -37,6 +38,8 @@ public class ModuleContext {
 
     private boolean isEarlyCallInitializationCompleted = true;
 
+    private JsonNode extUserOptable;
+
     private String id5Signature;
 
     public static ModuleContext of(AuctionInvocationContext invocationContext) {

extra/modules/optable-targeting/src/main/java/org/prebid/server/hooks/modules/optable/targeting/model/config/OptableTargetingProperties.java

@@ -29,6 +29,9 @@ public final class OptableTargetingProperties {
 
     Long timeout;
 
+    @JsonProperty("api-timeout")
+    Long apiTimeout;
+
     @JsonProperty("id-prefix-order")
     String idPrefixOrder;
 

extra/modules/optable-targeting/src/main/java/org/prebid/server/hooks/modules/optable/targeting/v1/OptableRawAuctionRequestHook.java

@@ -44,10 +44,31 @@ public OptableRawAuctionRequestHook(ConfigResolver configResolver,
     public Future<InvocationResult<AuctionRequestPayload>> call(AuctionRequestPayload payload,
                                                                 AuctionInvocationContext invocationContext) {
 
-        final OptableTargetingProperties properties = configResolver.resolve(invocationContext.accountConfig());
         final ModuleContext moduleContext = new ModuleContext();
         moduleContext.setEarlyNetworkCallEnabled(true);
         moduleContext.setCallTargetingAPITimestamp(System.currentTimeMillis());
+
+        // whatever goes wrong here, the cleaner has to be applied, or user.ext.optable ids reach the bidders
+        try {
+            return resolveTargetingFlow(payload, invocationContext, moduleContext);
+        } catch (RuntimeException e) {
+            conditionalLogger.error("Failed to initiate Optable targeting call: " + e.getMessage(), logSamplingRate);
+
+            moduleContext.setEarlyCallInitializationCompleted(true);
+            moduleContext.setExtUserOptable(null);
+            moduleContext.failWithExecutionTime(
+                    System.currentTimeMillis() - moduleContext.getCallTargetingAPITimestamp());
+
+            return update(AuctionRequestCleaner.instance(), moduleContext);
+        }
+    }
+
+    private Future<InvocationResult<AuctionRequestPayload>> resolveTargetingFlow(
+            AuctionRequestPayload payload,
+            AuctionInvocationContext invocationContext,
+            ModuleContext moduleContext) {
+
+        final OptableTargetingProperties properties = configResolver.resolve(invocationContext.accountConfig());
         moduleContext.setOptableTargetingProperties(properties);
 
         if (!PropertiesValidator.isValid(properties)) {
@@ -61,7 +82,7 @@ public Future<InvocationResult<AuctionRequestPayload>> call(AuctionRequestPayloa
         }
 
         return optableTargetingFlowResolver.resolveAsyncOptableTargetingFlow(
-                moduleContext, payload, invocationContext, properties, false);
+                moduleContext, payload, invocationContext, properties);
     }
 
     public static Future<InvocationResult<AuctionRequestPayload>> update(
@@ -77,16 +98,6 @@ public static Future<InvocationResult<AuctionRequestPayload>> update(
                         .build());
     }
 
-    public static Future<InvocationResult<AuctionRequestPayload>> success(ModuleContext moduleContext) {
-
-        return Future.succeededFuture(
-                InvocationResultImpl.<AuctionRequestPayload>builder()
-                        .status(InvocationStatus.success)
-                        .action(InvocationAction.no_action)
-                        .moduleContext(moduleContext)
-                        .build());
-    }
-
     @Override
     public String code() {
         return CODE;

extra/modules/optable-targeting/src/main/java/org/prebid/server/hooks/modules/optable/targeting/v1/OptableTargetingProcessedAuctionRequestHook.java

@@ -1,62 +1,59 @@
 package org.prebid.server.hooks.modules.optable.targeting.v1;
 
 import io.vertx.core.Future;
-import org.prebid.server.hooks.execution.v1.InvocationResultImpl;
 import org.prebid.server.hooks.modules.optable.targeting.model.ModuleContext;
 import org.prebid.server.hooks.modules.optable.targeting.model.config.OptableTargetingProperties;
 import org.prebid.server.hooks.modules.optable.targeting.v1.core.ConfigResolver;
 import org.prebid.server.hooks.modules.optable.targeting.v1.core.OptableTargetingFlowResolver;
-import org.prebid.server.hooks.v1.InvocationAction;
 import org.prebid.server.hooks.v1.InvocationResult;
-import org.prebid.server.hooks.v1.InvocationStatus;
 import org.prebid.server.hooks.v1.auction.AuctionInvocationContext;
 import org.prebid.server.hooks.v1.auction.AuctionRequestPayload;
 import org.prebid.server.hooks.v1.auction.ProcessedAuctionRequestHook;
+import org.prebid.server.log.ConditionalLogger;
+import org.prebid.server.log.LoggerFactory;
 
 import java.util.Objects;
 
 public class OptableTargetingProcessedAuctionRequestHook implements ProcessedAuctionRequestHook {
 
     public static final String CODE = "optable-targeting-processed-auction-request-hook";
 
+    private static final ConditionalLogger conditionalLogger = new ConditionalLogger(
+            LoggerFactory.getLogger(OptableTargetingProcessedAuctionRequestHook.class));
+
     private final ConfigResolver configResolver;
 
     private final OptableTargetingFlowResolver optableTargetingFlowResolver;
 
+    private final double logSamplingRate;
+
     public OptableTargetingProcessedAuctionRequestHook(ConfigResolver configResolver,
-                                                       OptableTargetingFlowResolver earlyOptableCallResolver) {
+                                                       OptableTargetingFlowResolver earlyOptableCallResolver,
+                                                       double logSamplingRate) {
 
         this.configResolver = Objects.requireNonNull(configResolver);
         this.optableTargetingFlowResolver = Objects.requireNonNull(earlyOptableCallResolver);
+        this.logSamplingRate = logSamplingRate;
     }
 
     @Override
     public Future<InvocationResult<AuctionRequestPayload>> call(AuctionRequestPayload auctionRequestPayload,
                                                                 AuctionInvocationContext invocationContext) {
 
         final ModuleContext moduleContext = ModuleContext.of(invocationContext);
-        final OptableTargetingProperties properties = configResolver.resolve(invocationContext.accountConfig());
 
-        if (moduleContext.isEarlyNetworkCallEnabled()) {
-            if (moduleContext.isEarlyCallInitializationCompleted()) {
-                return success(moduleContext);
-            } else {
-                return optableTargetingFlowResolver.resolveAsyncOptableTargetingFlow(
-                        moduleContext, auctionRequestPayload, invocationContext, properties, true);
-            }
+        // whatever goes wrong here, the cleaner has to be applied, or user.ext.optable ids reach the bidders
+        try {
+            final OptableTargetingProperties properties = configResolver.resolve(invocationContext.accountConfig());
+            return optableTargetingFlowResolver.resolveOptableTargetingFlow(
+                    auctionRequestPayload, invocationContext, moduleContext, properties);
+        } catch (RuntimeException e) {
+            conditionalLogger.error("Failed to initiate Optable targeting call: " + e.getMessage(), logSamplingRate);
+
+            moduleContext.setEarlyCallInitializationCompleted(true);
+            moduleContext.setExtUserOptable(null);
+            return optableTargetingFlowResolver.failed(moduleContext);
         }
-
-        return optableTargetingFlowResolver.resolveOptableTargetingFlow(
-                auctionRequestPayload, invocationContext, moduleContext, properties);
-    }
-
-    public static Future<InvocationResult<AuctionRequestPayload>> success(ModuleContext moduleContext) {
-        return Future.succeededFuture(
-                InvocationResultImpl.<AuctionRequestPayload>builder()
-                        .status(InvocationStatus.success)
-                        .action(InvocationAction.no_action)
-                        .moduleContext(moduleContext)
-                        .build());
     }
 
     @Override

extra/modules/optable-targeting/src/main/java/org/prebid/server/hooks/modules/optable/targeting/v1/core/BidderEnrichmentSampler.java

@@ -53,6 +53,10 @@ public Set<String> sample(BidRequest bidRequest, OptableTargetingProperties opta
                 .collect(Collectors.toSet());
     }
 
+    public boolean hasBidders(BidRequest bidRequest) {
+        return !extractUniqueBidders(bidRequest).isEmpty();
+    }
+
     private static int resolvePercentage(BidderAliases aliases, String bidder,
                                          int defaultEnrichmentPercentage,
                                          Map<String, Integer> bidderEnrichmentPercentage) {
@@ -86,8 +90,4 @@ private static Stream<String> extractImpBidders(ObjectNode impExt) {
                 .filter(Ortb2ImplicitParametersResolver::isImpExtBidder)
                 .filter(field -> impExt.get(field).isObject());
     }
-
-    public boolean hasBidders(BidRequest bidRequest) {
-        return !extractUniqueBidders(bidRequest).isEmpty();
-    }
 }