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

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

Problem Statement

Solution

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.
New integration tests added.
Modified or extended existing tests.
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.

m-nagarajan · 2025-05-13T06:45:55Z

services/venice-controller/build.gradle

@@ -34,6 +34,7 @@ dependencies {

  implementation project(':internal:venice-common')
  implementation project(':clients:venice-thin-client')
+  implementation project(':internal:venice-client-common')


why is this not under venice-common rather than under venice-client-common. We can fix this if needed separately, no AI for you on this.

m-nagarajan · 2025-05-13T06:49:17Z

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

 import java.util.HashMap;
 import java.util.Map;


 public class RepushJobRequest {
-  public static final String SCHEDULED_TRIGGER = "Scheduled";
-  public static final String MANUAL_TRIGGER = "Manual";
+  public static final RepushStoreTriggerSource SCHEDULED_TRIGGER = RepushStoreTriggerSource.SCHEDULED;


you can directly use RepushStoreTriggerSource.SCHEDULED or MANUAL where ever its needed or use the new member variable triggerSource. No need to have another static variable for this.

m-nagarajan · 2025-05-13T17:08:32Z

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

+package com.linkedin.venice.stats.dimensions;
+
+public enum LogCompactionSelectionReason implements VeniceDimensionInterface {
+  TIME_SINCE_LAST_VERSION_CREATION_EXCEEDS_THRESHOLD;


is there another reason which we are thinking to add in the future?

m-nagarajan · 2025-05-13T17:11:37Z

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

@@ -62,6 +67,11 @@ public class VeniceController {
  private static final Logger LOGGER = LogManager.getLogger(VeniceController.class);
  private static final String CONTROLLER_GRPC_SERVER_THREAD_NAME = "ControllerGrpcServer";
  static final String CONTROLLER_SERVICE_NAME = "venice-controller";
+  public static final String CONTROLLER_SERVICE_METRIC_PREFIX = "controller";
+  public static final Collection<MetricEntity> CONTROLLER_SERVICE_METRIC_ENTITIES = Collections.unmodifiableList(


why duplicated?

m-nagarajan · 2025-05-13T17:13:21Z

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

@@ -375,11 +378,15 @@ public class VeniceParentHelixAdmin implements Admin {
  private final Map<String, Map<String, ControllerClient>> newFabricControllerClientMap =
      new VeniceConcurrentHashMap<>();

+  /** Metrics */
+  private final Map<String, LogCompactionStats> logCompactionStatsMap = new HashMap<>();


lets use VeniceConcurrentHashMap

m-nagarajan · 2025-05-13T17:14:19Z

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

+    // initialise logCompactionStatsMap
+    for (String clusterName: multiClusterConfigs.getClusters()) {
+      if (multiClusterConfigs.getControllerConfig(clusterName).isLogCompactionEnabled()) {
+        logCompactionStatsMap.put(clusterName, new LogCompactionStats(metricsRepository, clusterName));


use putIfAbsent

Inside LogCompactionStats, I see we use MetricEntityStateGeneric, the MES that doesn't have a caching layer. In that case, can we just use the clusterName as a dimension when we record, instead of building per cluster stats?

Ideally yes, but we are adding per cluster stats for tehuti metrics to keep it similar to other controller stats.

m-nagarajan · 2025-05-13T17:29:00Z

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

+        .recordRepushStoreCall(
+            repushJobRequest.getStoreName(),
+            repushJobRequest.getTriggerSource(),
+            response.isError() ? VeniceResponseStatusCategory.FAIL : VeniceResponseStatusCategory.SUCCESS);


JOB_EXECUTION_STATUS was defined but never used. where is the plan to use this?

lluwm · 2025-05-15T17:16:54Z

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

@@ -38,10 +42,20 @@ public class LogCompactionService extends AbstractVeniceService {
  private final Admin admin;
  private final VeniceControllerMultiClusterConfig multiClusterConfigs;
  final ScheduledExecutorService executor;
+  private final Map<String, LogCompactionStats> statsMap;


I can't find who uses this map?

lluwm · 2025-05-15T17:28:00Z

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

+    // initialise logCompactionStatsMap
+    for (String clusterName: multiClusterConfigs.getClusters()) {
+      if (multiClusterConfigs.getControllerConfig(clusterName).isLogCompactionEnabled()) {
+        logCompactionStatsMap.put(clusterName, new LogCompactionStats(metricsRepository, clusterName));


Inside LogCompactionStats, I see we use MetricEntityStateGeneric, the MES that doesn't have a caching layer. In that case, can we just use the clusterName as a dimension when we record, instead of building per cluster stats?

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

for metric to record which stores are nominated for compaction

…& compaction

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

Whitney Deng added 26 commits May 23, 2025 14:55

ControllerMetricEntity enum class

08af724

[WIP] setup VeniceMetricsRepositoru for controller

0e02387

contains wip code

MetricsRepository -> VeniceMetricRepository in VeniceController and V…

7f976b6

…eniceControllerContext

import otel in controller build.gradle

afa726d

[wip] LogCompactionServiceStats.java

c979a9f

[WIP] Attributes builder

7655f14

LogCompactionService basic constructor

8f3d55b

add JOB_RUN_STATUS dimension

2a86ec0

add LOG_COMPACTION_SELECTION_REASON dimension

6b679d4

add VENICE_CONTROLLER_ENDPOINT dimension

d415a71

REPUSH_STORE_ENDPOINT_CALL_COUNT in ControllerMetricEntity

69c1743

rebase: LogCompactionService scoped to cluster level

e08f1e8

rename LogCompactionServiceStats -> LogCompactionStats

55fd989

rebase

1a39ced

LogCompactionStats extends AbstractVeniceStats

301ce62

delete ControllerEndpoints.java

e9f3afa

add MetricsRepository to VeniceParentHelixAdmin constructor

36d69bc

move repushStore endpoint to parent controller

7e92b84

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

add Optional<MetricsRepository> in AbstractTestVeniceParentHelixAdmin…

55f6247

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

add metricName in ControllerMetricEntity to streamline access to metr…

4915107

…ic name

LogCompactionStatsTest

86e4f32

nit delete testing print statement

1caf418

pass in LogCompactionStats to CompactionManager

80032f0

for metric to record which stores are nominated for compaction

add STORE_NOMINATED_FOR_SCHEDULED_COMPACTION metric entity

6d11f0b

add STORE_NOMINATED_FOR_SCHEDULED_COMPACTION to LogCompactionStats

2062312

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

Whitney Deng added 7 commits May 23, 2025 15:23

cleanup after rebase

a78dff5

add createGuage()

76be48e

nit format

dccf750

revert mistake

3b918b0

merge conflict

d48b396

add storeCurrentVersionNumber as metric dimension

d1c0a2f

emit storeNominatedForScheduledCompactionMetric for store nomination …

522ff6f

…& compaction

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

WhitneyDeng commented Apr 1, 2025

Uh oh!

m-nagarajan left a comment

Uh oh!

m-nagarajan May 13, 2025

Uh oh!

m-nagarajan May 13, 2025

Uh oh!

m-nagarajan May 13, 2025

Uh oh!

m-nagarajan May 13, 2025

Uh oh!

m-nagarajan May 13, 2025

Uh oh!

m-nagarajan May 13, 2025

Uh oh!

lluwm May 15, 2025

Uh oh!

m-nagarajan May 17, 2025

Uh oh!

m-nagarajan May 13, 2025

Uh oh!

lluwm May 15, 2025

Uh oh!

lluwm May 15, 2025

Uh oh!

Uh oh!

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

WhitneyDeng commented Apr 1, 2025

Problem Statement

Solution

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!