Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 78 additions & 6 deletions src/main/java/org/prebid/server/cache/CoreCacheService.java
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ public class CoreCacheService {
private static final Logger logger = LoggerFactory.getLogger(CoreCacheService.class);

private static final String BID_WURL_ATTRIBUTE = "wurl";
private static final String TRACE_INFO_SEPARATOR = "-";
private static final int MAX_DATACENTER_REGION_LENGTH = 4;

private final HttpClient httpClient;
private final URL endpointUrl;
Expand All @@ -78,13 +80,18 @@ public class CoreCacheService {
private final MultiMap cacheHeaders;
private final Map<String, List<String>> debugHeaders;

private final boolean appendTraceInfoToCacheId;
private final String datacenterRegion;

public CoreCacheService(
HttpClient httpClient,
URL endpointUrl,
String cachedAssetUrlTemplate,
long expectedCacheTimeMs,
String apiKey,
boolean isApiKeySecured,
boolean appendTraceInfoToCacheId,
String datacenterRegion,
VastModifier vastModifier,
EventsService eventsService,
Metrics metrics,
Expand All @@ -107,6 +114,9 @@ public CoreCacheService(
? HttpUtil.headers().add(HttpUtil.X_PBC_API_KEY_HEADER, Objects.requireNonNull(apiKey))
: HttpUtil.headers();
debugHeaders = HttpUtil.toDebugHeaders(cacheHeaders);

this.appendTraceInfoToCacheId = appendTraceInfoToCacheId;
this.datacenterRegion = normalizeDatacenterRegion(datacenterRegion);
}

public String getEndpointHost() {
Expand Down Expand Up @@ -138,7 +148,8 @@ public String cacheVideoDebugLog(CachedDebugLog cachedDebugLog, Integer videoCac
return cacheKey;
}

private CachedCreative makeDebugCacheCreative(CachedDebugLog videoCacheDebugLog, String hbCacheId,
private CachedCreative makeDebugCacheCreative(CachedDebugLog videoCacheDebugLog,
String hbCacheId,
Integer videoCacheTtl) {
final JsonNode value = mapper.mapper().valueToTree(videoCacheDebugLog.buildCacheBody());
videoCacheDebugLog.setCacheKey(hbCacheId);
Expand Down Expand Up @@ -211,6 +222,7 @@ private List<CachedCreative> updatePutObjects(List<BidPutObject> bidPutObjects,
.bidid(null)
.bidder(null)
.timestamp(null)
.key(resolveCacheKey(accountId, putObject.getKey()))
.value(vastModifier.modifyVastXml(isEventsEnabled,
allowedBidders,
putObject,
Expand Down Expand Up @@ -268,7 +280,8 @@ private Future<CacheServiceResult> doCacheOpenrtb(List<CacheBid> bids,
final List<CachedCreative> cachedCreatives = Stream.concat(
bids.stream().map(cacheBid ->
createJsonPutObjectOpenrtb(cacheBid, accountId, eventsContext)),
videoBids.stream().map(videoBid -> createXmlPutObjectOpenrtb(videoBid, requestId, hbCacheId)))
videoBids.stream().map(videoBid ->
createXmlPutObjectOpenrtb(videoBid, requestId, hbCacheId, accountId)))
.collect(Collectors.toCollection(ArrayList::new));

if (cachedCreatives.isEmpty()) {
Expand Down Expand Up @@ -385,26 +398,34 @@ private CachedCreative createJsonPutObjectOpenrtb(CacheBid cacheBid,
bidObjectNode.put(BID_WURL_ATTRIBUTE, eventUrl);
}

final String resolvedCacheKey = resolveCacheKey(accountId);

final BidPutObject payload = BidPutObject.builder()
.aid(eventsContext.getAuctionId())
.type("json")
.key(resolvedCacheKey)
.value(bidObjectNode)
.ttlseconds(cacheBid.getTtl())
.build();

return CachedCreative.of(payload, creativeSizeFromAdm(bid.getAdm()));
}

private CachedCreative createXmlPutObjectOpenrtb(CacheBid cacheBid, String requestId, String hbCacheId) {
private CachedCreative createXmlPutObjectOpenrtb(CacheBid cacheBid,
String requestId,
String hbCacheId,
String accountId) {
final BidInfo bidInfo = cacheBid.getBidInfo();
final Bid bid = bidInfo.getBid();
final String vastXml = bid.getAdm();

final String customCacheKey = resolveCustomCacheKey(hbCacheId, bidInfo.getCategory());
final String resolvedCacheKey = resolveCacheKey(accountId, hbCacheId);
final String customCacheKey = resolveCustomCacheKey(resolvedCacheKey, bidInfo.getCategory());

final BidPutObject payload = BidPutObject.builder()
.aid(requestId)
.type("xml")
.key(customCacheKey != null ? customCacheKey : resolvedCacheKey)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, move key decision making logic to resolveCacheKey function, so that it will truly resolve cache key.

.value(vastXml != null ? new TextNode(vastXml) : null)
.ttlseconds(cacheBid.getTtl())
.key(customCacheKey)
Expand Down Expand Up @@ -515,10 +536,21 @@ private static String resolveVideoBidUuid(String uuid, String hbCacheId) {
}

private void updateCreativeMetrics(String accountId, List<CachedCreative> cachedCreatives) {
for (final CachedCreative cachedCreative : cachedCreatives) {
for (CachedCreative cachedCreative : cachedCreatives) {
final MetricName creativeType = resolveCreativeTypeName(cachedCreative.getPayload());
final Integer creativeTtl = cachedCreative.getPayload().getTtlseconds() != null
? cachedCreative.getPayload().getTtlseconds()
: cachedCreative.getPayload().getExpiry();
Copy link
Collaborator

Choose a reason for hiding this comment

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

We tend to avoid duplicating long method call chains. This can be simplified to smth like:

final Payload payload = cachedCreative.getPayload();
final Integer creativeTtl = ObjectUtil.defaultIfNull(payload.getTtlSeconds(), payload.getExpiry());


if (creativeTtl != null) {
metrics.updateCacheCreativeTtl(accountId,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, move accountId to next line. Or, maybe, this can be one liner?

creativeTtl,
creativeType);
}

metrics.updateCacheCreativeSize(accountId,
cachedCreative.getSize(),
resolveCreativeTypeName(cachedCreative.getPayload()));
creativeType);
}
}

Expand Down Expand Up @@ -553,4 +585,44 @@ private BidCacheRequest toBidCacheRequest(List<CachedCreative> cachedCreatives)
.map(CachedCreative::getPayload)
.toList());
}

private String resolveCacheKey(String accountId) {
return resolveCacheKey(accountId, null);
}

// hbCacheId is accepted here to have a backwards-compatibility with video category mapping
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't comment code, please, remove.

private String resolveCacheKey(String accountId, String hbCacheId) {
if (!appendTraceInfoToCacheId) {
return hbCacheId;
}

// no need to have additional separator if datacenter name won't be added
final boolean isDatacenterNamePopulated = StringUtils.isNotBlank(datacenterRegion);
final int separatorCount = isDatacenterNamePopulated ? 2 : 1;
final int accountIdLength = accountId.length();
final int traceInfoLength = isDatacenterNamePopulated
? accountIdLength + datacenterRegion.length() + separatorCount
: accountIdLength + separatorCount;

final String cacheKey = hbCacheId != null ? hbCacheId : idGenerator.generateId();
if (cacheKey != null && traceInfoLength < cacheKey.length()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets invert if and return hbCacheId here. So that big chunk of the code will be without unnecessary indentation.

final String substring = cacheKey.substring(0, cacheKey.length() - traceInfoLength);
return isDatacenterNamePopulated
? accountId + TRACE_INFO_SEPARATOR + datacenterRegion + TRACE_INFO_SEPARATOR + substring
: accountId + TRACE_INFO_SEPARATOR + substring;
} else {
return hbCacheId;
}
}

private String normalizeDatacenterRegion(String datacenterRegion) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

static modifier

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Gimme sec

if (datacenterRegion == null) {
return null;
}

final String trimmedDatacenterRegion = datacenterRegion.trim();
return trimmedDatacenterRegion.length() > MAX_DATACENTER_REGION_LENGTH
? trimmedDatacenterRegion.substring(0, MAX_DATACENTER_REGION_LENGTH)
: trimmedDatacenterRegion;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package org.prebid.server.metric;

import com.codahale.metrics.MetricRegistry;

import java.util.Objects;
import java.util.function.Function;

public class CacheCreativeTtlMetrics extends UpdatableMetrics {

CacheCreativeTtlMetrics(MetricRegistry metricRegistry, CounterType counterType, String prefix) {
super(Objects.requireNonNull(metricRegistry),
Objects.requireNonNull(counterType),
nameCreator(Objects.requireNonNull(prefix)));
}

private static Function<MetricName, String> nameCreator(String prefix) {
return metricName -> "%s.creative_ttl.%s".formatted(prefix, metricName);
}
}
7 changes: 7 additions & 0 deletions src/main/java/org/prebid/server/metric/CacheMetrics.java
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ class CacheMetrics extends UpdatableMetrics {

private final RequestMetrics requestsMetrics;
private final CacheCreativeSizeMetrics cacheCreativeSizeMetrics;
private final CacheCreativeTtlMetrics cacheCreativeTtlMetrics;

CacheMetrics(MetricRegistry metricRegistry, CounterType counterType) {
super(
Expand All @@ -21,6 +22,7 @@ class CacheMetrics extends UpdatableMetrics {

requestsMetrics = new RequestMetrics(metricRegistry, counterType, createPrefix());
cacheCreativeSizeMetrics = new CacheCreativeSizeMetrics(metricRegistry, counterType, createPrefix());
cacheCreativeTtlMetrics = new CacheCreativeTtlMetrics(metricRegistry, counterType, createPrefix());
}

CacheMetrics(MetricRegistry metricRegistry, CounterType counterType, String prefix) {
Expand All @@ -31,6 +33,7 @@ class CacheMetrics extends UpdatableMetrics {

requestsMetrics = new RequestMetrics(metricRegistry, counterType, createPrefix(prefix));
cacheCreativeSizeMetrics = new CacheCreativeSizeMetrics(metricRegistry, counterType, createPrefix(prefix));
cacheCreativeTtlMetrics = new CacheCreativeTtlMetrics(metricRegistry, counterType, createPrefix(prefix));
}

private static String createPrefix(String prefix) {
Expand All @@ -52,4 +55,8 @@ RequestMetrics requests() {
CacheCreativeSizeMetrics creativeSize() {
return cacheCreativeSizeMetrics;
}

CacheCreativeTtlMetrics creativeTtl() {
return cacheCreativeTtlMetrics;
}
}
5 changes: 5 additions & 0 deletions src/main/java/org/prebid/server/metric/Metrics.java
Original file line number Diff line number Diff line change
Expand Up @@ -694,6 +694,11 @@ public void updateAccountModuleDurationMetric(Account account, String moduleCode
}
}

public void updateCacheCreativeTtl(String accountId, Integer creativeTtl, MetricName creativeType) {
cache().creativeTtl().updateHistogram(creativeType, creativeTtl);
forAccount(accountId).cache().creativeTtl().updateHistogram(creativeType, creativeTtl);
}

private static class HookMetricMapper {

private static final EnumMap<ExecutionStatus, MetricName> STATUS_TO_METRIC =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,8 @@ CoreCacheService cacheService(
@Value("${auction.cache.expected-request-time-ms}") long expectedCacheTimeMs,
@Value("${pbc.api.key:#{null}}") String apiKey,
@Value("${cache.api-key-secured:false}") boolean apiKeySecured,
@Value("${cache.append-trace-info-to-cache-id:false}") boolean appendTraceInfoToCacheId,
@Value("${datacenter-region:#{null}}") String datacenterRegion,
VastModifier vastModifier,
EventsService eventsService,
HttpClient httpClient,
Expand All @@ -178,6 +180,8 @@ CoreCacheService cacheService(
expectedCacheTimeMs,
apiKey,
apiKeySecured,
appendTraceInfoToCacheId,
datacenterRegion,
vastModifier,
eventsService,
metrics,
Expand Down
Loading
Loading