Conversation
This commit upgrades the Kargo Analytics Adapter from a minimal implementation to a comprehensive analytics solution. ## Changes ### Module (kargoAnalyticsAdapter.js) - Expanded from 2 events to 9 core Prebid events: - AUCTION_INIT, BID_REQUESTED, BID_RESPONSE, NO_BID - BID_TIMEOUT, BIDDER_DONE, BIDDER_ERROR, AUCTION_END, BID_WON - Now tracks ALL bidders, not just Kargo (competitive intelligence) - Implemented auction cache pattern for state management - Added Kargo-specific metrics: win/loss, margin to win, rank, response times - Privacy consent handling (GDPR, USP, GPP) with sanitized strings - Configurable sampling rate (1-100%) - New POST endpoints: /api/v2/analytics/auction and /api/v2/analytics/win - Graceful error handling throughout ### Tests (kargoAnalyticsAdapter_spec.js) - Comprehensive test suite with 31 test cases covering: - Adapter registration and configuration - All event handlers - Privacy consent extraction - Sampling behavior - Payload formatting - Error handling - Competitive metrics ### Technical Details - Uses established patterns from PubMatic/Magnite adapters - Event handler map for clean event routing - Delayed send to allow BID_WON events before auction data - Automatic cleanup of old auction cache entries gulp lint: PASSED gulp test --file kargoAnalyticsAdapter_spec.js: PASSED (31 tests)
Adds comprehensive API documentation for the Kargo Analytics Adapter endpoints including: - Complete TypeScript interfaces for all payloads - Example JSON payloads for both /auction and /win endpoints - Data considerations (sampling, currency, privacy) - Recommended database schema - Questions for data team discussion
WalkthroughReplaces the previous thin analytics adapter with a self-contained, event-driven Kargo analytics module: default config, in-memory auction/adUnit cache, comprehensive handlers for auction lifecycle events, payload formatting, sampling and consent logic, batched ajax delivery, and new public APIs (enableAnalytics, disableAnalytics, track). Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Bidder
participant Adapter as Kargo Analytics Adapter
participant Cache as State Cache
participant Server as Analytics Server
Client->>Adapter: enableAnalytics(config)
Adapter->>Adapter: validate config, determine sampling
Adapter->>Cache: init auction state
Client->>Adapter: track(AUCTION_INIT)
Adapter->>Cache: store auction & adUnit info
Client->>Adapter: track(BID_REQUESTED)
Adapter->>Cache: record bid request
Client->>Adapter: track(BID_RESPONSE)
Adapter->>Cache: store bid metrics (CPM, size, bidder)
Client->>Adapter: track(BID_TIMEOUT) / track(BIDDER_ERROR)
Adapter->>Cache: mark bidder timeout/error
Client->>Adapter: track(AUCTION_END)
Adapter->>Adapter: format auction payload (consent, metrics, adUnits)
Adapter->>Server: sendAuctionAnalytics(payload)
Server-->>Adapter: ACK
Adapter->>Cache: cleanup auction
Client->>Adapter: track(BID_WON)
Adapter->>Adapter: format win payload (winner + competitors)
Adapter->>Server: sendWinAnalytics(payload)
Server-->>Adapter: ACK
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Keeping documentation local for team review only.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @modules/kargoAnalyticsAdapter.js:
- Around line 418-422: The loop that marks bids won uses a too-broad condition
(bid.bidId === requestId || bid.bidder === bidderCode) which can set multiple
bids to won; update the logic in the adUnit.bids iteration to match only by the
unique requestId (use bid.bidId === requestId) and remove the bidderCode check
so only the exact winning bid (identified by requestId) gets bid.won = true;
ensure you still iterate Object.values(adUnit.bids) and set the won flag on the
matching bid only.
- Around line 318-325: The code may throw if adUnit.bids[bid.bidId] is undefined
because you assign cachedBid.serverResponseTime without checking cachedBid;
update the block around cachedBid/adUnit.bids to first verify cachedBid is
truthy (e.g., if (cachedBid) { ... }) before touching its properties, and move
the serverResponseTime assignment inside that guard so references to
cachedBid.serverResponseTime occur only when cachedBid exists; keep the existing
status update logic (cachedBid.status = 'no-bid') and the serverResponseTime
assignment (cachedBid.serverResponseTime = bid.serverResponseTimeMs) inside the
same checked branch.
In @test/spec/modules/kargoAnalyticsAdapter_spec.js:
- Around line 419-430: The test uses sampling: 0 which your adapter coerces to
100, so change the test to use sampling: 1 and stub Math.random to a
deterministic value (e.g., return 0.5) so shouldSample (Math.random() * 100 <
samplingRate) reliably returns false and no requests are sent; update the spec
to stub Math.random before enabling kargoAnalyticsAdapter and restore it after
the test, or alternatively change the assertion to verify the validation
behavior by checking that _config.sampling is reset to 100 and a warning was
logged.
🧹 Nitpick comments (1)
modules/kargoAnalyticsAdapter.js (1)
45-63: Consider simplifying the number formatting.The pattern
parseFloat(Number(cpm).toFixed(3))can be simplified.Number(cpm).toFixed(3)already returns a string, andparseFloatconverts it back to a number. Consider using a helper or just returning the result directly if string is acceptable.♻️ Optional simplification
- return parseFloat(Number(cpm).toFixed(3)); + return Math.round(cpm * 1000) / 1000;This avoids string conversion overhead and handles floating-point precision more directly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
modules/kargoAnalyticsAdapter.jstest/spec/modules/kargoAnalyticsAdapter_spec.js
🧰 Additional context used
🧬 Code graph analysis (2)
test/spec/modules/kargoAnalyticsAdapter_spec.js (1)
modules/kargoAnalyticsAdapter.js (7)
kargoAnalyticsAdapter(666-714)payload(595-595)payload(634-634)adUnit(232-232)adUnit(274-274)adUnit(416-416)adUnit(548-548)
modules/kargoAnalyticsAdapter.js (2)
src/utils.js (1)
logWarn(229-235)test/spec/modules/kargoAnalyticsAdapter_spec.js (17)
payload(249-249)payload(262-262)payload(275-275)payload(290-290)payload(304-304)payload(341-341)payload(358-358)payload(376-376)payload(389-389)payload(399-399)payload(446-446)payload(480-480)payload(498-498)payload(532-532)payload(552-552)payload(563-563)adapter(123-123)
🔇 Additional comments (34)
test/spec/modules/kargoAnalyticsAdapter_spec.js (12)
1-7: LGTM!Imports are correctly structured with the analytics adapter, testing utilities (chai, sinon server mock), events constants, and adapterManager for integration testing.
12-19: LGTM!The
defaultAdapterConfigis well-structured for testing withsendDelay: 0to avoid timing issues in unit tests andsampling: 100to ensure all events are captured.
110-119: LGTM!Test hooks properly initialize fake timers and stub
events.getEvents, with correct cleanup inafterEachto prevent test pollution.
121-127: LGTM!Integration test correctly verifies the adapter registration with
adapterManagerusing the 'kargo' provider code.
129-139: LGTM!Smoke tests for
enableAnalyticsverify that valid configurations and minimal configurations are accepted without throwing errors.
141-231: LGTM!Event handling tests correctly verify that individual events (AUCTION_INIT, BID_REQUESTED, BID_RESPONSE, NO_BID, BID_TIMEOUT, BIDDER_ERROR) are batched and don't trigger immediate server requests.
233-326: LGTM!AUCTION_END tests thoroughly validate:
- Analytics transmission timing with fake timers
- Correct v2 endpoint URL and HTTP method
- Payload structure including auctionId, timeout, version, duration
- Kargo-specific metrics, auction summary, and ad unit breakdown
- Duplicate send prevention via
sentflag
328-362: LGTM!BID_WON tests correctly verify that win events are sent immediately to the
/api/v2/analytics/winendpoint, with proper payload structure including winner details and Kargo participation data.
365-403: LGTM!Privacy consent tests correctly verify that:
- GDPR consent is extracted with
appliesflag and sanitizedconsentString('[present]')- USP consent string is extracted
- Raw consent strings are NOT transmitted (privacy-preserving design)
433-501: LGTM!Payload formatting tests comprehensively validate:
- Required fields (version, timestamp, auctionId, auctionTimeout, pageUrl)
- Multi-bidder scenarios in ad unit breakdown
- Graceful handling of empty or malformed data
503-538: LGTM!Error handling tests verify graceful handling of events without
auctionIdand proper capture of bidder errors in the payload.
540-577: LGTM!Competitive metrics tests verify that response times, CPM values, and average response time calculations are correctly captured and formatted in the analytics payload.
modules/kargoAnalyticsAdapter.js (22)
1-16: LGTM!Imports and constants are well-organized with clear naming conventions and appropriate module dependencies.
24-31: LGTM!State management uses a cache object pattern with proper initialization. The module-level state (
_config,_sampled) is reset indisableAnalytics, ensuring clean test isolation.
37-40: LGTM!The sampling logic correctly uses a random threshold comparison. The fallback to 100% ensures sampling always works even if config is malformed.
65-82: LGTM!The
extractSizesfunction correctly handles both banner and video media types, with proper unwrapping of nested videoplayerSizearrays.
84-119: LGTM!Privacy consent extraction properly sanitizes GDPR and GPP consent strings to
'[present]'for privacy compliance, while preserving the structured USP consent format (which is a standardized 4-character string, not PII).
121-134: LGTM!The
calculateRankfunction correctly computes bid rank by sorting CPMs in descending order and returning a 1-based position.
136-151: LGTM!Helper functions
countTotalBidsandaverageare implemented correctly with proper null/NaN handling and defensive coding.
153-193: LGTM!
handleAuctionInitproperly initializes the auction cache with all necessary fields, extracts consent from bidder requests, and sets up ad unit tracking structures.
195-218: LGTM!
handleBidRequestedcorrectly tracks bid requests per ad unit with proper null guards and initializes bid entries with pending status.
220-263: LGTM!
handleBidResponsecaptures comprehensive bid data with proper fallback handling fororiginalRequestId/requestIdand defensive slicing ofadvertiserDomains.
265-280: LGTM!
handleNoBidcorrectly updates bid status and tracks no-bid responses.
282-302: LGTM!
handleBidTimeoutcorrectly handles the array-based timeout event structure and updates bid statuses accordingly.
331-359: LGTM!
handleBidderErrorproperly captures error details with fallbackauctionIdresolution and correctly marks affected bids as errored.
361-395: LGTM!
handleAuctionEndcorrectly finalizes auction data, retrieves highest CPM bids with error handling, and schedules delayed transmission to accommodate BID_WON events.
431-443: LGTM!The event handler map cleanly associates all nine Prebid events with their respective handlers.
445-482: LGTM!
extractKargoMetricsprovides comprehensive competitive intelligence for Kargo bids including margin-to-win calculations and response time averages.
484-539: LGTM!
formatAuctionPayloadproduces a well-structured analytics payload with comprehensive auction data, Kargo metrics, and per-ad-unit breakdowns.
541-577: LGTM!
formatWinPayloadcorrectly formats win event data with proper null handling and conditional Kargo participation metrics.
579-622: LGTM!
sendAuctionAnalyticsimplements proper error handling with cleanup on both success and failure paths, and respects the sampling configuration.
624-650: LGTM!
sendWinAnalyticsuses a fire-and-forget pattern appropriate for individual win events, with proper sampling and null checks.
652-660: LGTM!The 30-second cleanup delay is appropriate to accommodate late-arriving BID_WON events while preventing unbounded memory growth.
662-723: LGTM!The adapter definition follows Prebid.js patterns correctly with proper configuration merging, validation, event tracking with error handling, and clean adapter registration.
| const cachedBid = adUnit.bids[bid.bidId]; | ||
| if (cachedBid && cachedBid.status === 'pending') { | ||
| cachedBid.status = 'no-bid'; | ||
| } | ||
| case EVENTS.BID_RESPONSE: { | ||
| handleBidResponseData(args); | ||
| break; | ||
| // Capture server response time if available | ||
| if (typeof bid.serverResponseTimeMs !== 'undefined') { | ||
| cachedBid.serverResponseTime = bid.serverResponseTimeMs; | ||
| } |
There was a problem hiding this comment.
Potential null reference when accessing cachedBid.serverResponseTime.
On line 323, bid.serverResponseTimeMs is checked, but cachedBid.serverResponseTime is assigned without verifying that cachedBid exists. If cachedBid is undefined (e.g., adUnit.bids[bid.bidId] doesn't exist), this will throw a TypeError.
🐛 Proposed fix
if (cachedBid && cachedBid.status === 'pending') {
cachedBid.status = 'no-bid';
}
// Capture server response time if available
- if (typeof bid.serverResponseTimeMs !== 'undefined') {
+ if (cachedBid && typeof bid.serverResponseTimeMs !== 'undefined') {
cachedBid.serverResponseTime = bid.serverResponseTimeMs;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const cachedBid = adUnit.bids[bid.bidId]; | |
| if (cachedBid && cachedBid.status === 'pending') { | |
| cachedBid.status = 'no-bid'; | |
| } | |
| case EVENTS.BID_RESPONSE: { | |
| handleBidResponseData(args); | |
| break; | |
| // Capture server response time if available | |
| if (typeof bid.serverResponseTimeMs !== 'undefined') { | |
| cachedBid.serverResponseTime = bid.serverResponseTimeMs; | |
| } | |
| const cachedBid = adUnit.bids[bid.bidId]; | |
| if (cachedBid && cachedBid.status === 'pending') { | |
| cachedBid.status = 'no-bid'; | |
| } | |
| // Capture server response time if available | |
| if (cachedBid && typeof bid.serverResponseTimeMs !== 'undefined') { | |
| cachedBid.serverResponseTime = bid.serverResponseTimeMs; | |
| } |
🤖 Prompt for AI Agents
In @modules/kargoAnalyticsAdapter.js around lines 318 - 325, The code may throw
if adUnit.bids[bid.bidId] is undefined because you assign
cachedBid.serverResponseTime without checking cachedBid; update the block around
cachedBid/adUnit.bids to first verify cachedBid is truthy (e.g., if (cachedBid)
{ ... }) before touching its properties, and move the serverResponseTime
assignment inside that guard so references to cachedBid.serverResponseTime occur
only when cachedBid exists; keep the existing status update logic
(cachedBid.status = 'no-bid') and the serverResponseTime assignment
(cachedBid.serverResponseTime = bid.serverResponseTimeMs) inside the same
checked branch.
| Object.values(adUnit.bids).forEach(bid => { | ||
| if (bid.bidId === requestId || bid.bidder === bidderCode) { | ||
| bid.won = true; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Overly broad bid matching may incorrectly mark multiple bids as won.
The condition bid.bidId === requestId || bid.bidder === bidderCode could mark multiple bids from the same bidder as "won" if they participated in multiple ad units. Consider matching by requestId only, since that uniquely identifies the winning bid.
🐛 Proposed fix
Object.values(adUnit.bids).forEach(bid => {
- if (bid.bidId === requestId || bid.bidder === bidderCode) {
+ if (bid.bidId === requestId) {
bid.won = true;
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Object.values(adUnit.bids).forEach(bid => { | |
| if (bid.bidId === requestId || bid.bidder === bidderCode) { | |
| bid.won = true; | |
| } | |
| }); | |
| Object.values(adUnit.bids).forEach(bid => { | |
| if (bid.bidId === requestId) { | |
| bid.won = true; | |
| } | |
| }); |
🤖 Prompt for AI Agents
In @modules/kargoAnalyticsAdapter.js around lines 418 - 422, The loop that marks
bids won uses a too-broad condition (bid.bidId === requestId || bid.bidder ===
bidderCode) which can set multiple bids to won; update the logic in the
adUnit.bids iteration to match only by the unique requestId (use bid.bidId ===
requestId) and remove the bidderCode check so only the exact winning bid
(identified by requestId) gets bid.won = true; ensure you still iterate
Object.values(adUnit.bids) and set the won flag on the matching bid only.
| it('should respect 0% sampling rate', function () { | ||
| kargoAnalyticsAdapter.enableAnalytics({ | ||
| provider: 'kargo', | ||
| options: { sampling: 0, sendDelay: 0 } | ||
| }); | ||
|
|
||
| events.emit(EVENTS.AUCTION_INIT, { ...mockAuctionInit, auctionId: 'new-auction-1' }); | ||
| events.emit(EVENTS.AUCTION_END, { auctionId: 'new-auction-1' }); | ||
| clock.tick(1000); | ||
|
|
||
| expect(server.requests.length).to.equal(0); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the kargoAnalyticsAdapter.js file
fd -n "kargoAnalyticsAdapter\.js" --type f | grep -v specRepository: KargoGlobal/Prebid.js
Length of output: 295
🏁 Script executed:
# Search for the sampling validation logic and enableAnalytics implementation
rg -n -A 10 "enableAnalytics\s*[=:]|sampling.*<\s*1|Invalid sampling" modules/kargoAnalyticsAdapter.js | head -100Repository: KargoGlobal/Prebid.js
Length of output: 412
🏁 Script executed:
# Also check what the test file imports and verify the test setup
rg -n -B 5 -A 15 "should respect 0% sampling rate" test/spec/modules/kargoAnalyticsAdapter_spec.jsRepository: KargoGlobal/Prebid.js
Length of output: 754
🏁 Script executed:
# Find the shouldSample function
rg -n -B 3 -A 10 "shouldSample\s*[=:].*function|function.*shouldSample" modules/kargoAnalyticsAdapter.jsRepository: KargoGlobal/Prebid.js
Length of output: 459
🏁 Script executed:
# Check the test beforeEach/afterEach setup
rg -n -B 20 "should respect 0% sampling rate" test/spec/modules/kargoAnalyticsAdapter_spec.js | head -50Repository: KargoGlobal/Prebid.js
Length of output: 857
🏁 Script executed:
# Look for any mocking of Math.random in the test file
rg -n "Math\.random|stub.*random" test/spec/modules/kargoAnalyticsAdapter_spec.js | head -20Repository: KargoGlobal/Prebid.js
Length of output: 47
🏁 Script executed:
# Check how _sampled is used in event handlers
rg -n "_sampled" modules/kargoAnalyticsAdapter.js | head -20Repository: KargoGlobal/Prebid.js
Length of output: 235
🏁 Script executed:
# Look at AUCTION_END handler to see if it respects sampling
rg -n -B 5 -A 15 "EVENTS\.AUCTION_END|'auction:end'" modules/kargoAnalyticsAdapter.js | head -50Repository: KargoGlobal/Prebid.js
Length of output: 825
🏁 Script executed:
# Check if there's any early return based on _sampled
rg -n -B 2 -A 5 "if.*_sampled|if.*!_sampled" modules/kargoAnalyticsAdapter.jsRepository: KargoGlobal/Prebid.js
Length of output: 402
Sampling rate of 0% is invalid and gets reset to 100%.
The adapter validates that sampling must be between 1-100 (line 679: if (_config.sampling < 1 || _config.sampling > 100)). When sampling: 0 is passed, this condition triggers, logging a warning and resetting _config.sampling to 100. The test then expects 0 requests, but shouldSample() (line 39: Math.random() * 100 < samplingRate) will always return true with 100% sampling, causing events to be processed and requests to be sent.
This test does not actually test 0% sampling behavior as intended. Use sampling: 1 with a deterministic stub on Math.random() to reliably test low sampling rates, or revise the test to verify the validation reset behavior instead.
🤖 Prompt for AI Agents
In @test/spec/modules/kargoAnalyticsAdapter_spec.js around lines 419 - 430, The
test uses sampling: 0 which your adapter coerces to 100, so change the test to
use sampling: 1 and stub Math.random to a deterministic value (e.g., return 0.5)
so shouldSample (Math.random() * 100 < samplingRate) reliably returns false and
no requests are sent; update the spec to stub Math.random before enabling
kargoAnalyticsAdapter and restore it after the test, or alternatively change the
assertion to verify the validation behavior by checking that _config.sampling is
reset to 100 and a warning was logged.
nickllerandi
changed the title
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@modules/kargoAnalyticsAdapter.js`:
- Line 731: The forEach callback currently uses an expression body which
implicitly returns a value; change the callback for
Object.keys(cache.auctions).forEach to use a block body so it does not return
anything — e.g., replace the single-expression arrow callback that performs
delete cache.auctions[key] with a brace-wrapped function body containing the
delete statement (and semicolon) so the callback returns undefined; target the
line using Object.keys(cache.auctions).forEach and the delete
cache.auctions[key] expression.
| _config = { ...DEFAULT_CONFIG }; | ||
| _sampled = true; | ||
| // Clear cache | ||
| Object.keys(cache.auctions).forEach(key => delete cache.auctions[key]); |
There was a problem hiding this comment.
Avoid returning a value from the forEach callback (Biome lint).
Wrap the delete in a block so the callback doesn’t return a value.
♻️ Proposed fix
- Object.keys(cache.auctions).forEach(key => delete cache.auctions[key]);
+ Object.keys(cache.auctions).forEach(key => {
+ delete cache.auctions[key];
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Object.keys(cache.auctions).forEach(key => delete cache.auctions[key]); | |
| Object.keys(cache.auctions).forEach(key => { | |
| delete cache.auctions[key]; | |
| }); |
🧰 Tools
🪛 Biome (2.3.13)
[error] 731-731: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In `@modules/kargoAnalyticsAdapter.js` at line 731, The forEach callback currently
uses an expression body which implicitly returns a value; change the callback
for Object.keys(cache.auctions).forEach to use a block body so it does not
return anything — e.g., replace the single-expression arrow callback that
performs delete cache.auctions[key] with a brace-wrapped function body
containing the delete statement (and semicolon) so the callback returns
undefined; target the line using Object.keys(cache.auctions).forEach and the
delete cache.auctions[key] expression.
1 participant
Type of change
Bugfix
Feature
New bidder adapter
Updated bidder adapter
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Does this change affect user-facing APIs or examples documented on http://prebid.org?
Other
Description of change
Other information
Summary by CodeRabbit
New Features
Refactor
Tests