[da-vinci][server] Global RT DIV: Add OTel metrics for end-to-end feature observability - #2658
[da-vinci][server] Global RT DIV: Add OTel metrics for end-to-end feature observability#2658KaiSernLim wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds end-to-end OpenTelemetry (OTel) observability for the Global RT DIV best-effort propagation lifecycle, enabling operators to measure leader send/payload size, follower persist, VT position sync, and categorized errors without changing ingestion behavior.
Changes:
- Introduces a new metrics dimension (
VENICE_GLOBAL_RT_DIV_ERROR_TYPE) and enum (VeniceGlobalRtDivErrorType:SEND,PERSIST,VT_SYNC,DELETE). - Registers 5 new ingestion OTel metric entities and adds corresponding recording APIs in
IngestionOtelStats(+ no-op overrides, + AggVersioned router methods). - Extends unit tests to validate the new dimension naming, metric entity registration, and end-to-end metric recording via in-memory OTel readers.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/venice-client-common/src/main/java/com/linkedin/venice/stats/dimensions/VeniceMetricsDimensions.java | Adds VENICE_GLOBAL_RT_DIV_ERROR_TYPE dimension key. |
| internal/venice-client-common/src/main/java/com/linkedin/venice/stats/dimensions/VeniceGlobalRtDivErrorType.java | New enum implementing VeniceDimensionInterface for Global RT DIV error categorization. |
| internal/venice-client-common/src/test/java/com/linkedin/venice/stats/dimensions/VeniceMetricsDimensionsTest.java | Verifies naming transforms (snake/camel/pascal) for the new dimension. |
| internal/venice-client-common/src/test/java/com/linkedin/venice/stats/dimensions/VeniceGlobalRtDivErrorTypeTest.java | New unit test validating enum-to-dimension wiring and expected values. |
| clients/da-vinci-client/src/main/java/com/linkedin/davinci/stats/ingestion/IngestionOtelMetricEntity.java | Registers 5 new Global RT DIV ingestion OTel metrics. |
| clients/da-vinci-client/src/main/java/com/linkedin/davinci/stats/ingestion/IngestionOtelStats.java | Adds metric state initialization + 4 recording methods for Global RT DIV. |
| clients/da-vinci-client/src/main/java/com/linkedin/davinci/stats/ingestion/NoOpIngestionOtelStats.java | Adds no-op overrides for the new Global RT DIV recording methods. |
| clients/da-vinci-client/src/main/java/com/linkedin/davinci/stats/AggVersionedIngestionStats.java | Adds routing methods so call sites can record the new metrics per store/version. |
| clients/da-vinci-client/src/test/java/com/linkedin/davinci/stats/ingestion/IngestionOtelMetricEntityTest.java | Updates expected metric entity registry with the 5 new entities + dimensions. |
| clients/da-vinci-client/src/test/java/com/linkedin/davinci/stats/ingestion/IngestionOtelStatsTest.java | Adds tests validating all Global RT DIV metrics (counter + histogram + error dimension). |
| clients/da-vinci-client/src/test/java/com/linkedin/davinci/stats/ServerMetricEntityTest.java | Updates expected total metric entity count to include the 5 new metrics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi there. This pull request has been inactive for 30 days. To keep our review queue healthy, we plan to close it in 7 days unless there is new activity. If you are still working on this, please push a commit, leave a comment, or convert it to draft to signal intent. Thank you for your time and contributions. |
|
Closing this pull request due to 37 days of inactivity. This is not a judgment on the value of the work. If you would like to continue, please reopen or open a new PR and we will be happy to take another look. Thank you again for contributing. |
…ture observability Adds seven OTel metric entities covering every phase of the Global RT DIV lifecycle (send, persist, vt_sync, delete, load), along with two new dimensions: venice.global_rt_div.error.type and venice.global_rt_div.load.outcome. All call sites are wired in LeaderFollowerStoreIngestionTask and StoreIngestionTask. Phases that currently propagate exceptions (persist, delete, vt_sync) record the error and rethrow, so failures become observable without changing existing behavior. Phases that are best-effort (send compression, load read/deserialize) record alongside the existing log. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1bd6623 to
3742d5b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
internal/venice-client-common/src/main/java/com/linkedin/venice/stats/dimensions/VeniceGlobalRtDivLoadOutcome.java:19
- The NOT_FOUND enum value’s Javadoc says the leader “starts from EARLIEST”, but the leader-start behavior when no Global RT DIV state is loaded is determined by the normal leader start logic (and can fall back to a rewind-based start), not necessarily EARLIEST. This comment is misleading for operators interpreting the metric dimension.
/** No usable state read from disk; leader starts from EARLIEST */
internal/venice-client-common/src/main/java/com/linkedin/venice/stats/dimensions/VeniceMetricsDimensions.java:126
- This dimension’s Javadoc says it’s for “best-effort operations”, but the error type is used for all Global RT DIV operation failures (including phases that record-and-rethrow like PERSIST/DELETE/VT_SYNC). The current wording is inaccurate and could confuse readers about which failures are expected to be swallowed vs propagated.
/** {@link VeniceGlobalRtDivErrorType} Error type for Global RT DIV best-effort operations */
VENICE_GLOBAL_RT_DIV_ERROR_TYPE("venice.global_rt_div.error.type"),
|
🤖 Responding to github-actions bot notices 4317549461 and 4362649401: declined as code feedback, since both are automated PR lifecycle notices and contain no code review findings. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Problem Statement
The Global RT DIV feature (leader→follower RT DIV state propagation) has zero metrics today. Several of its phases are best-effort — they catch and log rather than fail ingestion — so operators have no way to verify:
Solution
Adds 7 OTel metrics covering the full lifecycle, with all call sites wired in, so the feature is observable end to end the moment
server.global.rt.div.enabledis turned on.ingestion.global_rt_div.send_sizecountaggregation doubles as the send countingestion.global_rt_div.send_rt_producer_countingestion.global_rt_div.persist_countingestion.global_rt_div.vt_sync_producer_countOffsetRecord. Thecountaggregation doubles as the sync countingestion.global_rt_div.error_countSEND/PERSIST/VT_SYNC/DELETE/LOADingestion.global_rt_div.load_countFOUND/NOT_FOUNDingestion.global_rt_div.load_rt_producer_countAll metrics carry
VENICE_STORE_NAME,VENICE_CLUSTER_NAME, andVENICE_VERSION_ROLE.How to use them: the
send_sizecount vspersist_countratio across leader and follower replicas detects drops. A zerovt_sync_producer_countcount alongside a non-zeropersist_countmeans the offset checkpointing path is broken.load_count{NOT_FOUND}is expected for the first leader ever elected for a partition, but a sustained rate is not. Any non-zeroerror_countis directly actionable.New dimensions:
venice.global_rt_div.error.type→VeniceGlobalRtDivErrorType(SEND,PERSIST,VT_SYNC,DELETE,LOAD)venice.global_rt_div.load.outcome→VeniceGlobalRtDivLoadOutcome(FOUND,NOT_FOUND)LOADexists as an error type becausereadGlobalRtDivStateanddeserializeGlobalRtDivStateboth returnnullon failure, which is indistinguishable from "genuinely absent" at theloadGlobalRtDivcall site.load_count{NOT_FOUND}therefore covers both, anderror_count{LOAD}is the signal that disambiguates them.Call-site error handling: phases that currently propagate exceptions (
PERSIST,DELETE,VT_SYNC) record the error and rethrow, so behavior is preserved exactly while failures become observable — storage failures are deliberately not swallowed. Phases that already catch-and-log (send compression, load read/deserialize) simply record alongside the existing log.Infrastructure added:
VeniceGlobalRtDivErrorTypeandVeniceGlobalRtDivLoadOutcomeenums + their keys inVeniceMetricsDimensionsIngestionOtelMetricEntityconstantsIngestionOtelStats+ no-op overrides inNoOpIngestionOtelStatsAggVersionedIngestionStatsLeaderFollowerStoreIngestionTask(send, load) andStoreIngestionTask(persist, delete, vt_sync)Code changes
server.global.rt.div.enabledconfig (defaultfalse), since the code paths themselves only execute when the feature is on.Concurrency-Specific Checks
Both reviewer and PR author to verify
record-and-rethrowpattern preserves the existing propagation semantics on every path it touches.How was this PR tested?
VeniceGlobalRtDivErrorTypeTestandVeniceGlobalRtDivLoadOutcomeTestvalidate the new enums. New test methods inIngestionOtelStatsTestcover all 7 metrics end-to-end using in-memory OTel readers.IngestionOtelMetricEntityTest(7 new entities),ServerMetricEntityTest(count 185→192),VeniceMetricsDimensionsTest(2 new dimensions),LeaderFollowerStoreIngestionTaskTest(asserts the persist and load-error metrics actually fire).Does this PR introduce any user-facing or breaking changes?