Add per-table insert/update counters to upsert metadata managers#18427
Add per-table insert/update counters to upsert metadata managers#18427priyen-stripe wants to merge 3 commits intoapache:masterfrom
Conversation
…metrics Emits per-table counters in both ConcurrentMapPartitionUpsertMetadataManager and its consistent-deletes variant to distinguish append (new PK) from update (existing PK) traffic in upsert tables. Out-of-order records are counted by neither metric, consistent with the existing UPSERT_OUT_OF_ORDER meter. Prometheus metric names: pinot_server_upsertNewKeysInserted pinot_server_upsertExistingKeysUpdated Co-authored-by: Priyen Patel <priyenpatel2014@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18427 +/- ##
============================================
- Coverage 63.61% 63.59% -0.02%
Complexity 1717 1717
============================================
Files 3252 3252
Lines 199051 199146 +95
Branches 30838 30876 +38
============================================
+ Hits 126618 126652 +34
- Misses 62352 62418 +66
+ Partials 10081 10076 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
xiangfu0
left a comment
xiangfu0
left a comment
There was a problem hiding this comment.
Found two high-signal metric-contract issues; see inline comments.
| } | ||
| }); | ||
|
|
||
| if (isNewKey.get()) { |
There was a problem hiding this comment.
This block increments the new meters for every non-out-of-order record, including tombstones. On delete-enabled upsert tables, a first-seen delete will hit UPSERT_NEW_KEYS_INSERTED and a delete for an existing PK will hit UPSERT_EXISTING_KEYS_UPDATED, even though neither is an insert/update row. Because these are new exported Prometheus counters, that makes the public metric contract wrong for delete-heavy workloads. Please guard these counters with !recordInfo.isDeleteRecord() or add a separate delete meter.
There was a problem hiding this comment.
good catch, pushed commit to fix this
| } | ||
| }); | ||
|
|
||
| if (isNewKey.get()) { |
There was a problem hiding this comment.
Same issue here: delete records are counted under the new insert/update meters. In delete-enabled tables this misclassifies tombstone traffic as inserts/updates, so the exposed per-table counters become incorrect even though the underlying upsert state is handled correctly. This needs the same !recordInfo.isDeleteRecord() guard or a dedicated delete counter.
There was a problem hiding this comment.
good catch, pushed commit to fix this
…meter Address PR feedback: - Guard insert/update metrics against delete records by moving metric emission inline next to addDocId/replaceDocId inside the compute() lambda. - Add dedicated UPSERT_KEYS_DELETED counter for tombstone ingestion. - Remove isNewKey AtomicBoolean since it's no longer needed. Co-authored-by: Priyen Patel <priyenpatel2014@gmail.com>
priyen
force-pushed
the
more-upsert-metrics
branch
from
0e17219 to
03d4f3e
Compare
Consolidate duplicated upsert metric emission into a shared BasePartitionUpsertMetadataManager.emitUpsertMetrics() method. Rename UPSERT_NEW_KEYS_INSERTED/UPSERT_EXISTING_KEYS_UPDATED to UPSERT_KEYS_INSERTED/UPSERT_KEYS_UPDATED for consistent naming. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Committed-By-Agent: claude
|
@xiangfu0 thanks for the review, address your feedback |
3 participants
Summary
UPSERT_KEYS_INSERTED,UPSERT_KEYS_UPDATED, andUPSERT_KEYS_DELETEDtoServerMeterto distinguish append, update, and delete traffic in upsert tablesConcurrentHashMap.compute()lambda, directly next toaddDocId/replaceDocId, in bothConcurrentMapPartitionUpsertMetadataManagerandConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletesUPSERT_KEYS_DELETEDinstead of the insert/update counters, keeping the metric contract correct for delete-heavy workloadsUPSERT_OUT_OF_ORDERmeterisNewKeyAtomicBoolean — the lambda structure itself determines which metric to emit, same ashandleOutOfOrderEventwhich already emits metrics inside the lambdaNew metric names:
Testing
Tested internally at Stripe on test tables, worked:
Label
observability