Core: Improve traceability for Prebid Cache stores #3757
Conversation
Net-burst
changed the title
Net-burst
force-pushed
the
prebid-cache-traceability
branch
2 times, most recently
from
402df05 to
055bca0
Compare
Net-burst
force-pushed
the
prebid-cache-traceability
branch
from
055bca0 to
363a50f
Compare
Net-burst
changed the title
| : accountIdLength + separatorCount; | ||
|
|
||
| final String cacheKey = hbCacheId != null ? hbCacheId : idGenerator.generateId(); | ||
| if (cacheKey != null && traceInfoLength < cacheKey.length()) { |
There was a problem hiding this comment.
lets invert if and return hbCacheId here. So that big chunk of the code will be without unnecessary indentation.
| return resolveCacheKey(accountId, null); | ||
| } | ||
|
|
||
| // hbCacheId is accepted here to have a backwards-compatibility with video category mapping |
There was a problem hiding this comment.
We don't comment code, please, remove.
| final String customCacheKey = resolveCustomCacheKey(resolvedCacheKey, bidInfo.getCategory()); | ||
|
|
||
| final BidPutObject payload = BidPutObject.builder() | ||
| .aid(requestId) | ||
| .type("xml") | ||
| .key(customCacheKey != null ? customCacheKey : resolvedCacheKey) |
There was a problem hiding this comment.
Please, move key decision making logic to resolveCacheKey function, so that it will truly resolve cache key.
| .bidid(null) | ||
| .bidder(null) | ||
| .timestamp(null) |
There was a problem hiding this comment.
No need to populate null fields, even for clarification purposes. We don't do that + its also clear that if you don't include field in the builder, field will be null.
| final BidPutObject secondBidPutObject = BidPutObject.builder() | ||
| .type("xml") | ||
| .bidid("bidId2") | ||
| .bidder("bidder2") | ||
| .timestamp(1L) | ||
| .value(new TextNode("VAST")) | ||
| .build(); | ||
| final BidPutObject thirdBidPutObject = BidPutObject.builder() | ||
| .type("text") | ||
| .bidid("bidId3") | ||
| .bidder("bidder3") | ||
| .timestamp(1L) | ||
| .value(new TextNode("VAST")) | ||
| .build(); |
There was a problem hiding this comment.
No need to test three objects, one will be sufficient. Here you test cache id functionality and not accompanying logic. Please, leave single put object to simplify test. And, as always, please, check for similar occurences.
| final Integer creativeTtl = cachedCreative.getPayload().getTtlseconds() != null | ||
| ? cachedCreative.getPayload().getTtlseconds() | ||
| : cachedCreative.getPayload().getExpiry(); |
There was a problem hiding this comment.
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());| : cachedCreative.getPayload().getExpiry(); | ||
|
|
||
| if (creativeTtl != null) { | ||
| metrics.updateCacheCreativeTtl(accountId, |
There was a problem hiding this comment.
Please, move accountId to next line. Or, maybe, this can be one liner?
| : accountId + TRACE_INFO_SEPARATOR + substring; | ||
| } | ||
|
|
||
| private String normalizeDatacenterRegion(String datacenterRegion) { |
* Add functional tests for prebid cache trace ability * update test * Update after review * Update after review * Update after review
🔧 Type of changes
✨ What's the context?
Prebid Cache lacks the information about which account is storing the creative and can't emit any account metrics.
🧠 Rationale behind the change
This is a part of the effort to increase PBC traceability.
🔎 New Bid Adapter Checklist
🧪 Test plan
How do you know the changes are safe to ship to production?
🏎 Quality check