[controller] integrate OpenTelemetry into Controller & add log compaction metrics #1665

WhitneyDeng · 2025-04-01T03:17:59Z

Problem Statement

integrating OpenTelemetry into controllers
adding metrics for log compaction

Solution

OpenTelemetry integration into controller

add OpenTelemetry dependency to venice-controller/build.gradle
VeniceControllerWrapper metricsRepository initialised with VeniceMetricsRepository

Log Compaction logic change

VeniceParentHelixAdmin::getStoresForCompaction() calls VeniceHelixAdmin::getStoresForCompaction() rather than throw exception

Metric Type added:

Gauge (DoubleGauge)

Metrics added in `LogCompactionStats`:

Metric Name	Metric Description	Metric Type	Dimensions (VENICE_CLUSTER_NAME is a base dimension)
`REPUSH_CALL_COUNT`	Repush API call count	Counter	VENICE_STORE_NAME, VENICE_RESPONSE_STATUS_CODE_CATEGORY, VENICE_CLUSTER_NAME, REPUSH_TRIGGER_SOURCE
`COMPACTION_ELIGIBLE_STATE`	Duration a store is nominated for compaction before repush triggered. Value set to 1: when store is nominated for compaction. Value set to 0: when repush on the store succeeds.	Gauge	VENICE_STORE_NAME, VENICE_CLUSTER_NAME
`STORE_NOMINATED_FOR_COMPACTION_COUNT`	Number of times a store is nominated for compaction	Counter	VENICE_STORE_NAME, VENICE_CLUSTER_NAME

Metric Dimension Added

REPUSH_TRIGGER_SOURCE for REPUSH_CALL_COUNT

Other OTel changes

VeniceOpenTelemetryMetricsRepository::getFullMetricName take MetricEntity instead of String prefix name and metric name

Code changes

Added new code behind a config. If so list the config names and their default values in the PR description.
Introduced new log lines.
- Confirmed if logs need to be rate limited to avoid excessive logging.

Concurrency-Specific Checks

Both reviewer and PR author to verify

Code has no race conditions or thread safety issues.
Proper synchronization mechanisms (e.g., synchronized, RWLock) are used where needed.
No blocking calls inside critical sections that could lead to deadlocks or performance degradation.
Verified thread-safe collections are used (e.g., ConcurrentHashMap, CopyOnWriteArrayList).
Validated proper exception handling in multi-threaded code to avoid silent thread termination.

How was this PR tested?

New unit tests added.
- LogCompactionStatsTest to test LogCompactionStats metric emission
New integration tests added.
Modified or extended existing tests.
- VeniceOpenTelemetryMetricsRepositoryTest: getFullMetricName & Gauge changes
- VeniceMetricsDimensionsTest: add REPUSH_TRIGGER_SOURCE
Verified backward compatibility (if applicable).

Does this PR introduce any user-facing or breaking changes?

No. You can skip the rest of this section.
Yes. Clearly explain the behavior change and its impact.

m-nagarajan

Thanks. Left some very initial review.

services/venice-controller/build.gradle

.../venice-controller/src/main/java/com/linkedin/venice/controller/repush/RepushJobRequest.java

...-common/src/main/java/com/linkedin/venice/stats/dimensions/LogCompactionSelectionReason.java

services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceController.java

...s/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceParentHelixAdmin.java

...troller/src/main/java/com/linkedin/venice/controller/logcompaction/LogCompactionService.java

...s/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceParentHelixAdmin.java

...al/venice-client-common/src/main/java/com/linkedin/venice/stats/dimensions/JobRunStatus.java

...ce-controller/src/main/java/com/linkedin/venice/controller/stats/ControllerMetricEntity.java

...venice-controller/src/main/java/com/linkedin/venice/controller/stats/LogCompactionStats.java

contains wip code

…eniceControllerContext

- add RepushStoreTriggerSource enum for metric dimension - import `venice-client-common` in `venice-controller` to allow `VeniceParentHelixAdmin` to access `VeniceResponseStatusCategory` to emit metric - add clusterName to RepushJobRequest for metric dimension - in RepushJobRequest, streamline triggerSource to `RepushStoreTriggerSource`

- VeniceParentHelixAdmin::getStoresForCompaction call VeniceHelixAdmin::getStoresForCompaction directly rather than throw exception - VeniceController only creates LogCompactionService if is parent controller

…::initializeParentAdmin to pass in metricsRepository for testing metric emission in VeniceParentHelixAdmin

…ic name

...ent-common/src/main/java/com/linkedin/venice/stats/VeniceOpenTelemetryMetricsRepository.java

...lient-common/src/main/java/com/linkedin/venice/stats/dimensions/VeniceMetricsDimensions.java

internal/venice-client-common/src/main/java/com/linkedin/venice/stats/metrics/MetricType.java

.../src/integrationTest/java/com/linkedin/venice/integration/utils/VeniceControllerWrapper.java

services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceController.java

...troller/src/main/java/com/linkedin/venice/controller/logcompaction/LogCompactionService.java

...ce-controller/src/main/java/com/linkedin/venice/controller/stats/ControllerMetricEntity.java

...venice-controller/src/main/java/com/linkedin/venice/controller/stats/LogCompactionStats.java

…licating getMetricPrefix(metricEntity), metricEntity.getMetricName() for all metric types

… `storeNominationToCompactionCompleteDurationMetric` emission from repush trigger logic

… `storeNominationToCompactionCompleteDurationMetric` metric description

…MPACTION_ELIGIBLE_STATE`

…s/failure

…ics attribute

…to-controller

m-nagarajan

Left a few comments. Thanks for reiterating. Can you also add a detailed PR description with the changes involved for stats including the metrics being added and also any other non-stats related log compaction related changes in the PR.

internal/venice-client-common/src/main/java/com/linkedin/venice/stats/metrics/MetricType.java

services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java

...venice-controller/src/main/java/com/linkedin/venice/controller/stats/LogCompactionStats.java

…ePushStatusInfo)

- nixed STORE_COMPACTION_TRIGGER_STATUS - move recordRepushStoreCall emission from VeniceParentHelixAdmin to common path in VeniceHelixAdmin - problem is to mock all relevant components in VeniceHelixAdmin for testing in LogCompactionStatsTest

WhitneyDeng changed the title ~~[controller] integrate OpenTelemetry into Controller~~ [controller] integrate OpenTelemetry into Controller & add log compaction metrics May 8, 2025

m-nagarajan reviewed May 13, 2025

View reviewed changes

lluwm reviewed May 15, 2025

View reviewed changes

...troller/src/main/java/com/linkedin/venice/controller/logcompaction/LogCompactionService.java Outdated Show resolved Hide resolved

...s/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceParentHelixAdmin.java Outdated Show resolved Hide resolved

WhitneyDeng force-pushed the log_compaction/observability/integrate-otel-to-controller branch from 6797aeb to 2062312 Compare May 23, 2025 22:11

m-nagarajan reviewed Jun 9, 2025

View reviewed changes

WhitneyDeng mentioned this pull request Jun 23, 2025

[log-compaction][controller] log LogCompactionService failure to start, nix exception to prevent controller crash #1893

Closed

14 tasks

WhitneyDeng marked this pull request as ready for review June 24, 2025 22:50

Whitney Deng added 23 commits June 26, 2025 11:22

ControllerMetricEntity enum class

1df9788

[WIP] setup VeniceMetricsRepositoru for controller

1f9509a

contains wip code

MetricsRepository -> VeniceMetricRepository in VeniceController and V…

52bc641

…eniceControllerContext

import otel in controller build.gradle

635eb84

[wip] LogCompactionServiceStats.java

80c61f6

[WIP] Attributes builder

5f4c377

LogCompactionService basic constructor

f178b0f

add JOB_RUN_STATUS dimension

f24ff9b

add LOG_COMPACTION_SELECTION_REASON dimension

972c64f

add VENICE_CONTROLLER_ENDPOINT dimension

5b5dab1

REPUSH_STORE_ENDPOINT_CALL_COUNT in ControllerMetricEntity

1e16d89

rebase: LogCompactionService scoped to cluster level

bf91c0c

rename LogCompactionServiceStats -> LogCompactionStats

e64f1a0

rebase

888d88a

LogCompactionStats extends AbstractVeniceStats

9ff8279

delete ControllerEndpoints.java

f81819c

add MetricsRepository to VeniceParentHelixAdmin constructor

64addcc

move repushStore endpoint to parent controller

f7954c6

- VeniceParentHelixAdmin::getStoresForCompaction call VeniceHelixAdmin::getStoresForCompaction directly rather than throw exception - VeniceController only creates LogCompactionService if is parent controller

add Optional<MetricsRepository> in AbstractTestVeniceParentHelixAdmin…

5831ea2

…::initializeParentAdmin to pass in metricsRepository for testing metric emission in VeniceParentHelixAdmin

add metricName in ControllerMetricEntity to streamline access to metr…

2680725

…ic name

LogCompactionStatsTest

99b6c64

nit delete testing print statement

1b1390b

m-nagarajan reviewed Jun 27, 2025

View reviewed changes

...venice-controller/src/main/java/com/linkedin/venice/controller/stats/LogCompactionStats.java Outdated Show resolved Hide resolved

Whitney Deng and others added 17 commits June 27, 2025 11:52

[pr] pass in just metricEntity to getFullMetricName() rather than dup…

0509b31

…licating getMetricPrefix(metricEntity), metricEntity.getMetricName() for all metric types

[pr] gauge metric type description

1b18714

[pr] ModuleMetricEntityInterface.getUniqueMetricEntities

2b3bc6e

[pr] ternary operator for propertiesList null check

5201afe

[pr] clean up storeNominationToCompactionTriggeredDurationMetric ->…

c6cbf3b

… `storeNominationToCompactionCompleteDurationMetric` emission from repush trigger logic

[pr] clean up storeNominationToCompactionTriggeredDurationMetric ->…

754340e

… `storeNominationToCompactionCompleteDurationMetric` metric description

[pr] rename repush.store.trigger.source -> repush.trigger.source

4ab2483

[pr] rename REPUSH_STORE_ENDPOINT_CALL_COUNT -> REPUSH_CALL_COUNT

c362436

[pr] rename STORE_NOMINATION_TO_COMPACTION_COMPLETE_DURATION -> `CO…

874120b

…MPACTION_ELIGIBLE_STATE`

[pr] rename STORE_NOMINATION_TO_COMPACTION_COMPLETE_DURATION -> `CO…

daa3a77

…MPACTION_ELIGIBLE_STATE`

[pr] STORE_COMPACTION_TRIGGER_STATUS records trigger attempt succes…

c9f85ab

…s/failure

nit: log message

874e34f

[PR] toggle off COMPACTION_ELIGIBLE_STATE on successful push only

1143b85

[PR] reuse cluster name & store name

08935c0

gh check: fix test, add venice repush request status to required metr…

5f010a4

…ics attribute

check pushjob is repush to mark log compaction complete

d98d9b1

Merge branch 'main' into log_compaction/observability/integrate-otel-…

abd0443

…to-controller

m-nagarajan reviewed Jul 8, 2025

View reviewed changes

Whitney Deng added 8 commits July 7, 2025 18:57

[pr] add documentation reference

50f2835

[pr] only initialise Stats method if log compaction enabled

6feb429

[gh] fix prefix test

91877dc

[pr] fix double braces

3d153be

[pr] directly get pushId rather than in config

5fbc1f5

[pr] ObservableDoubleGauge comment

80605f0

[pr] move logCompactionStatsMap to Admin (rather than Admin::Offlin…

ca130e4

…ePushStatusInfo)

[gh check] try to fix test:

0a166ee

- nixed STORE_COMPACTION_TRIGGER_STATUS - move recordRepushStoreCall emission from VeniceParentHelixAdmin to common path in VeniceHelixAdmin - problem is to mock all relevant components in VeniceHelixAdmin for testing in LogCompactionStatsTest

pthirun closed this Jul 14, 2025

pthirun mentioned this pull request Jul 21, 2025

[controller] Log compaction/otel integration #1922

Merged

17 tasks

[controller] integrate OpenTelemetry into Controller & add log compaction metrics #1665

[controller] integrate OpenTelemetry into Controller & add log compaction metrics #1665

Uh oh!

Conversation

WhitneyDeng commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem Statement

Solution

OpenTelemetry integration into controller

Log Compaction logic change

Metric Type added:

Metrics added in LogCompactionStats:

Metric Dimension Added

Other OTel changes

Code changes

Concurrency-Specific Checks

How was this PR tested?

Does this PR introduce any user-facing or breaking changes?

Uh oh!

m-nagarajan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

m-nagarajan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

WhitneyDeng commented Apr 1, 2025 •

edited

Loading

Metrics added in `LogCompactionStats`: