Skip to content

Commit 556ac58

Browse files
committed
[server][da-vinci] Fix negative OTel counters by using cumulative sum() in
async counter callbacks The ObservableLongCounter callback in MetricEntityState.reportToMeasurement() used LongAdder.sumThenReset() which reports deltas. The OTel SDK expects cumulative values from async counter callbacks and computes its own deltas internally (current - previous). This caused delta-of-delta, producing negative counter values when traffic varied between collection intervals. Fix: replace sumThenReset() with sum() so the callback reports cumulative values. Remove sumThenReset() from MetricAttributesData entirely. Add negative-value guard for ASYNC_COUNTER_FOR_HIGH_PERF_CASES (silently dropped, matching OTel SDK behavior for synchronous LongCounter). Add reusable multi-collection test utilities in OpenTelemetryDataTestUtils for both ASYNC_COUNTER and ASYNC_UP_DOWN_COUNTER, validating DELTA and CUMULATIVE temporality. Add regression tests across DIVStats, KafkaConsumerServiceStats, IngestionOtelStats, and ServerReadQuotaUsageStats.
1 parent 413d87d commit 556ac58

8 files changed

Lines changed: 602 additions & 38 deletions

File tree

clients/da-vinci-client/src/test/java/com/linkedin/davinci/stats/DIVStatsOtelTest.java

Lines changed: 39 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -214,7 +214,7 @@ public void testDIVResultDimensionIsolation() {
214214
stats.recordSuccessMsg(TEST_STORE_NAME, 1);
215215
stats.recordDuplicateMsg(TEST_STORE_NAME, 1);
216216

217-
// Collect once — ASYNC_COUNTER uses sumThenReset, so subsequent collections would see 0.
217+
// Collect once to snapshot all dimension values from a single collection interval.
218218
Collection<MetricData> metrics = inMemoryMetricReader.collectAllMetrics();
219219
validateAsyncCounterFromCollection(
220220
metrics,
@@ -549,9 +549,7 @@ private void validateAsyncCounter(String metricName, long expectedValue, Attribu
549549

550550
/**
551551
* Validates an async counter value from a pre-collected metrics snapshot. Use this when
552-
* validating multiple dimension values of the same ASYNC_COUNTER metric in a single test —
553-
* each {@code collectAllMetrics()} call resets the LongAdder via {@code sumThenReset()},
554-
* so only the first collection returns the accumulated values.
552+
* validating multiple dimension values of the same ASYNC_COUNTER metric in a single test.
555553
*/
556554
private static void validateAsyncCounterFromCollection(
557555
Collection<MetricData> metricsData,
@@ -564,4 +562,41 @@ private static void validateAsyncCounterFromCollection(
564562
assertEquals(point.getValue(), expectedValue);
565563
}
566564

565+
/**
566+
* Verifies that DIV MESSAGE_COUNT (ASYNC_COUNTER_FOR_HIGH_PERF_CASES) produces correct data
567+
* across multiple collection intervals under both DELTA and CUMULATIVE temporality.
568+
*/
569+
@Test
570+
public void testMessageCountMultiCollection() {
571+
Attributes attrs = buildMessageAttributes(TEST_STORE_NAME, VersionRole.CURRENT, VeniceDIVResult.SUCCESS);
572+
OpenTelemetryDataTestUtils.validateAsyncCounterMultiCollection(
573+
TEST_METRIC_PREFIX,
574+
SERVER_METRIC_ENTITIES,
575+
OTEL_MESSAGE_COUNT,
576+
attrs,
577+
repo -> {
578+
AggVersionedDIVStats s = createDIVStatsWithStore(repo, TEST_STORE_NAME);
579+
return n -> {
580+
for (int i = 0; i < n; i++)
581+
s.recordSuccessMsg(TEST_STORE_NAME, 1);
582+
};
583+
},
584+
new long[] { 5, 2, 8 });
585+
}
586+
587+
/** Creates an AggVersionedDIVStats with a single store set up (version 1=CURRENT). */
588+
private AggVersionedDIVStats createDIVStatsWithStore(VeniceMetricsRepository repo, String storeName) {
589+
ReadOnlyStoreRepository mockRepo = mock(ReadOnlyStoreRepository.class);
590+
doReturn(Collections.emptyList()).when(mockRepo).getAllStores();
591+
AggVersionedDIVStats divStats = new AggVersionedDIVStats(repo, mockRepo, true, TEST_CLUSTER_NAME);
592+
Store mockStore = mock(Store.class);
593+
doReturn(storeName).when(mockStore).getName();
594+
doReturn(1).when(mockStore).getCurrentVersion();
595+
Version v1 = new VersionImpl(storeName, 1, "push-1");
596+
v1.setStatus(VersionStatus.ONLINE);
597+
doReturn(Collections.singletonList(v1)).when(mockStore).getVersions();
598+
divStats.handleStoreCreated(mockStore);
599+
return divStats;
600+
}
601+
567602
}

clients/da-vinci-client/src/test/java/com/linkedin/davinci/stats/KafkaConsumerServiceStatsOtelTest.java

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -769,4 +769,33 @@ private double getTehutiMetricValue(String statsName, String sensorName, String
769769
assertNotNull(metric, "Tehuti metric should exist: " + metricName);
770770
return metric.value();
771771
}
772+
773+
/**
774+
* Verifies that POLL_COUNT (ASYNC_COUNTER_FOR_HIGH_PERF_CASES) produces correct data
775+
* across multiple collection intervals under both DELTA and CUMULATIVE temporality.
776+
*/
777+
@Test
778+
public void testPollCountMultiCollection() {
779+
OpenTelemetryDataTestUtils.validateAsyncCounterMultiCollection(
780+
TEST_METRIC_PREFIX,
781+
SERVER_METRIC_ENTITIES,
782+
POLL_COUNT.getMetricName(),
783+
buildNonStoreAttributes(),
784+
repo -> {
785+
KafkaConsumerServiceStats s = new KafkaConsumerServiceStats(
786+
repo,
787+
TOTAL_STORE_NAME,
788+
() -> 42L,
789+
null,
790+
SystemTime.INSTANCE,
791+
TEST_CLUSTER_NAME,
792+
TEST_REGION_NAME,
793+
TEST_POOL_TYPE);
794+
return n -> {
795+
for (int i = 0; i < n; i++)
796+
s.recordPollRequestLatency(5.0);
797+
};
798+
},
799+
new long[] { 10, 4, 15 });
800+
}
772801
}

clients/da-vinci-client/src/test/java/com/linkedin/davinci/stats/ingestion/IngestionOtelStatsTest.java

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@
8888
import com.linkedin.venice.stats.dimensions.VenicePartialUpdateOperation;
8989
import com.linkedin.venice.stats.dimensions.VeniceRecordType;
9090
import com.linkedin.venice.stats.dimensions.VeniceRegionLocality;
91+
import com.linkedin.venice.utils.OpenTelemetryDataTestUtils;
9192
import io.opentelemetry.api.common.Attributes;
9293
import io.opentelemetry.sdk.testing.exporter.InMemoryMetricReader;
9394
import io.tehuti.metrics.MetricsRepository;
@@ -1367,4 +1368,23 @@ private Map<Integer, Integer> getPushTimeoutByVersion(IngestionOtelStats stats)
13671368
private interface ReflectiveResolveBackup {
13681369
int resolve() throws Exception;
13691370
}
1371+
1372+
/**
1373+
* Verifies that ingestion ASYNC_COUNTER_FOR_HIGH_PERF_CASES metrics produce correct data
1374+
* across multiple collection intervals under both DELTA and CUMULATIVE temporality.
1375+
*/
1376+
@Test
1377+
public void testRecordsConsumedMultiCollection() {
1378+
OpenTelemetryDataTestUtils.validateAsyncCounterMultiCollection(
1379+
TEST_PREFIX,
1380+
SERVER_METRIC_ENTITIES,
1381+
INGESTION_RECORDS_CONSUMED.getMetricEntity().getMetricName(),
1382+
buildAttributesWithVersionRoleAndReplicaType(VersionRole.CURRENT, ReplicaType.LEADER),
1383+
repo -> {
1384+
IngestionOtelStats s = createStats(repo);
1385+
s.updateVersionInfo(CURRENT_VERSION, FUTURE_VERSION);
1386+
return n -> s.recordRecordsConsumed(CURRENT_VERSION, ReplicaType.LEADER, (int) n);
1387+
},
1388+
new long[] { 500, 100, 800 });
1389+
}
13701390
}

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

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
*
1616
* <p>The {@link LongAdder} provides high-throughput recording capability by minimizing contention
1717
* across threads. The accumulated value is read during OpenTelemetry's metric collection callback
18-
* via {@link #sumThenReset()}.
18+
* via {@link #sum()} (for ObservableLongCounter/UpDownCounter callbacks that must report cumulative values).
1919
*/
2020
public class MetricAttributesData {
2121
private final Attributes attributes;
@@ -67,13 +67,20 @@ public void add(long value) {
6767
}
6868

6969
/**
70-
* Returns the current sum and resets the adder to zero.
71-
* This is typically called during OpenTelemetry's metric collection callback.
72-
* Only call this for observable counter metrics where adder is guaranteed non-null.
70+
* Returns the current cumulative sum without resetting.
71+
* This is the correct method to use in OpenTelemetry's ObservableLongCounter/UpDownCounter callbacks,
72+
* which must report <b>cumulative</b> values per the OTel spec. The SDK handles delta computation
73+
* internally based on the configured aggregation temporality.
74+
*
75+
* <p>Using {@code sumThenReset()} in observable counter callbacks causes the SDK to compute
76+
* delta-of-delta (because it subtracts the previous observation from the current one, but
77+
* {@code sumThenReset()} already returns a delta), producing negative counter values when
78+
* traffic varies between collection intervals.
7379
*
74-
* @return the sum before reset
80+
* @return the current cumulative sum
7581
*/
76-
public long sumThenReset() {
77-
return adder.sumThenReset();
82+
public long sum() {
83+
return adder.sum();
7884
}
85+
7986
}

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

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
*/
2828
public abstract class MetricEntityState extends AsyncMetricEntityState {
2929
private final boolean isObservableCounter;
30+
private final boolean isMonotonicCounter;
3031
/** define both long and double consumer to avoid unnecessary conversions **/
3132
private final ObjDoubleConsumer<MetricAttributesData> otelDoubleRecordingStrategy;
3233
private final ObjLongConsumer<MetricAttributesData> otelLongRecordingStrategy;
@@ -49,6 +50,7 @@ public MetricEntityState(
4950
null);
5051
MetricType metricType = metricEntity.getMetricType();
5152
this.isObservableCounter = metricType.isObservableCounterType();
53+
this.isMonotonicCounter = metricType == MetricType.ASYNC_COUNTER_FOR_HIGH_PERF_CASES;
5254
this.otelDoubleRecordingStrategy = createOtelDoubleRecordingStrategy(metricType);
5355
this.otelLongRecordingStrategy = createOtelLongRecordingStrategy(metricType);
5456
}
@@ -86,6 +88,12 @@ protected final void registerObservableCounterIfNeeded() {
8688
/**
8789
* Reports all accumulated values to the OpenTelemetry measurement.
8890
* This is the callback invoked by OTel during metric collection.
91+
*
92+
* <p>Uses {@link MetricAttributesData#sum()} (not {@code sumThenReset()}) because the OTel spec
93+
* requires ObservableLongCounter/UpDownCounter callbacks to report <b>cumulative</b> values. The
94+
* SDK handles delta computation internally based on the configured aggregation temporality.
95+
* Using {@code sumThenReset()} caused the SDK to compute delta-of-delta, producing negative
96+
* counter values when traffic varied between collection intervals.
8997
*/
9098
private void reportToMeasurement(ObservableLongMeasurement measurement) {
9199
Iterable<MetricAttributesData> allData = getAllMetricAttributesData();
@@ -95,12 +103,15 @@ private void reportToMeasurement(ObservableLongMeasurement measurement) {
95103

96104
for (MetricAttributesData holder: allData) {
97105
if (holder.hasAdder()) {
98-
long value = holder.sumThenReset();
99-
// Skip zero values to avoid polluting metrics with stale attribute combinations
100-
// (e.g., from deleted stores) rather than trying to clean up all the registered
101-
// callbacks which could be complex. For delta-temporality async counters, omitting
102-
// a zero report correctly means "no change in this period."
103-
if (value != 0) {
106+
long value = holder.sum();
107+
// For monotonic counters (ASYNC_COUNTER), skip attribute combinations that were never
108+
// recorded (cumulative sum == 0, since values are always non-negative). For stale
109+
// combinations (e.g., deleted stores), the cumulative sum stays constant and the SDK
110+
// correctly computes delta=0.
111+
// For up-down counters (ASYNC_UP_DOWN_COUNTER), always report — a cumulative sum of 0
112+
// is a legitimate value (e.g., all connections opened have been closed) and must be
113+
// observed by the SDK to compute the correct delta.
114+
if (value != 0 || !isMonotonicCounter) {
104115
measurement.record(value, holder.getAttributes());
105116
}
106117
}
@@ -132,6 +143,11 @@ private ObjDoubleConsumer<MetricAttributesData> createOtelDoubleRecordingStrateg
132143
private ObjLongConsumer<MetricAttributesData> createOtelLongRecordingStrategy(MetricType metricType) {
133144
switch (metricType) {
134145
case ASYNC_COUNTER_FOR_HIGH_PERF_CASES:
146+
return (holder, value) -> {
147+
if (value >= 0) {
148+
holder.add(value);
149+
}
150+
};
135151
case ASYNC_UP_DOWN_COUNTER_FOR_HIGH_PERF_CASES:
136152
return (holder, value) -> holder.add(value);
137153
case COUNTER:

0 commit comments

Comments
 (0)