From df62060299c4483ceb905b983ce1b5dce1d4a0f8 Mon Sep 17 00:00:00 2001 From: Nisarg Thakkar Date: Tue, 10 Dec 2024 14:32:02 -0800 Subject: [PATCH] [server] Fix stats handling for SINGLE_GET requests (#1381) In a previous commit, when removing some metrics, we ended up removing some key metrics about key and value size values for single_get requests as well. That change also led to NPEs as the caller expected the K/V stats to always be recorded for Single-get requests. This commit adds Avg and Max stats by default for key and value sizes in single get requests. --- .../venice/stats/ServerHttpRequestStats.java | 24 +++++-- .../stats/AggServerHttpRequestStatsTest.java | 67 ++++++++++++++++--- 2 files changed, 73 insertions(+), 18 deletions(-) diff --git a/services/venice-server/src/main/java/com/linkedin/venice/stats/ServerHttpRequestStats.java b/services/venice-server/src/main/java/com/linkedin/venice/stats/ServerHttpRequestStats.java index 09f1c86a13..891dd05a26 100644 --- a/services/venice-server/src/main/java/com/linkedin/venice/stats/ServerHttpRequestStats.java +++ b/services/venice-server/src/main/java/com/linkedin/venice/stats/ServerHttpRequestStats.java @@ -295,23 +295,33 @@ public ServerHttpRequestStats( () -> totalStats.earlyTerminatedEarlyRequestCountSensor, new OccurrenceRate()); - if (isKeyValueProfilingEnabled) { - // size profiling is expensive + if (isKeyValueProfilingEnabled || requestType == RequestType.SINGLE_GET) { + final MeasurableStat[] valueSizeStats; + final MeasurableStat[] keySizeStats; String requestValueSizeSensorName = "request_value_size"; + String requestKeySizeSensorName = "request_key_size"; + if (isKeyValueProfilingEnabled) { + valueSizeStats = TehutiUtils + .getFineGrainedPercentileStatWithAvgAndMax(getName(), getFullMetricName(requestValueSizeSensorName)); + keySizeStats = TehutiUtils + .getFineGrainedPercentileStatWithAvgAndMax(getName(), getFullMetricName(requestKeySizeSensorName)); + } else { + valueSizeStats = new MeasurableStat[] { new Avg(), new Max() }; + keySizeStats = new MeasurableStat[] { new Avg(), new Max() }; + } + requestValueSizeSensor = registerPerStoreAndTotal( requestValueSizeSensorName, totalStats, () -> totalStats.requestValueSizeSensor, - TehutiUtils - .getFineGrainedPercentileStatWithAvgAndMax(getName(), getFullMetricName(requestValueSizeSensorName))); - String requestKeySizeSensorName = "request_key_size"; + valueSizeStats); requestKeySizeSensor = registerPerStoreAndTotal( requestKeySizeSensorName, totalStats, () -> totalStats.requestKeySizeSensor, - TehutiUtils - .getFineGrainedPercentileStatWithAvgAndMax(getName(), getFullMetricName(requestKeySizeSensorName))); + keySizeStats); } + misroutedStoreVersionSensor = registerPerStoreAndTotal( "misrouted_store_version_request_count", totalStats, diff --git a/services/venice-server/src/test/java/com/linkedin/venice/stats/AggServerHttpRequestStatsTest.java b/services/venice-server/src/test/java/com/linkedin/venice/stats/AggServerHttpRequestStatsTest.java index 338117f1c3..edee31e28b 100644 --- a/services/venice-server/src/test/java/com/linkedin/venice/stats/AggServerHttpRequestStatsTest.java +++ b/services/venice-server/src/test/java/com/linkedin/venice/stats/AggServerHttpRequestStatsTest.java @@ -13,10 +13,13 @@ public class AggServerHttpRequestStatsTest { - protected MetricsRepository metricsRepository; - protected MockTehutiReporter reporter; - protected AggServerHttpRequestStats singleGetStats; - protected AggServerHttpRequestStats batchGetStats; + private MetricsRepository metricsRepository; + private MockTehutiReporter reporter; + private MetricsRepository metricsRepositoryForKVProfiling; + private MockTehutiReporter reporterForKVProfiling; + private AggServerHttpRequestStats singleGetStats; + private AggServerHttpRequestStats singleGetStatsWithKVProfiling; + private AggServerHttpRequestStats batchGetStats; private static final String STORE_FOO = "store_foo"; private static final String STORE_BAR = "store_bar"; @@ -26,14 +29,21 @@ public class AggServerHttpRequestStatsTest { @BeforeTest public void setUp() { this.metricsRepository = new MetricsRepository(); - Assert.assertEquals(metricsRepository.metrics().size(), 0); this.reporter = new MockTehutiReporter(); this.metricsRepository.addReporter(reporter); + + this.metricsRepositoryForKVProfiling = new MetricsRepository(); + this.reporterForKVProfiling = new MockTehutiReporter(); + this.metricsRepositoryForKVProfiling.addReporter(reporterForKVProfiling); + + Assert.assertEquals(metricsRepository.metrics().size(), 0); + Assert.assertEquals(metricsRepositoryForKVProfiling.metrics().size(), 0); + this.singleGetStats = new AggServerHttpRequestStats( "test_cluster", metricsRepository, RequestType.SINGLE_GET, - true, + false, Mockito.mock(ReadOnlyStoreRepository.class), true, false); @@ -45,18 +55,31 @@ public void setUp() { Mockito.mock(ReadOnlyStoreRepository.class), true, false); + + this.singleGetStatsWithKVProfiling = new AggServerHttpRequestStats( + "test_cluster", + metricsRepositoryForKVProfiling, + RequestType.SINGLE_GET, + true, + Mockito.mock(ReadOnlyStoreRepository.class), + true, + false); } @AfterTest public void cleanUp() { metricsRepository.close(); + metricsRepositoryForKVProfiling.close(); reporter.close(); + reporterForKVProfiling.close(); } @Test public void testMetrics() { ServerHttpRequestStats singleGetServerStatsFoo = singleGetStats.getStoreStats(STORE_FOO); ServerHttpRequestStats singleGetServerStatsBar = singleGetStats.getStoreStats(STORE_BAR); + ServerHttpRequestStats singleGetServerStatsWithKvProfilingFoo = + singleGetStatsWithKVProfiling.getStoreStats(STORE_FOO); singleGetServerStatsFoo.recordSuccessRequest(); singleGetServerStatsFoo.recordSuccessRequest(); @@ -66,6 +89,9 @@ public void testMetrics() { singleGetServerStatsFoo.recordKeySizeInByte(100); singleGetServerStatsFoo.recordValueSizeInByte(1000); + singleGetServerStatsWithKvProfilingFoo.recordKeySizeInByte(100); + singleGetServerStatsWithKvProfilingFoo.recordValueSizeInByte(1000); + Assert.assertTrue( reporter.query("." + STORE_FOO + "--success_request.OccurrenceRate").value() > 0, "success_request rate should be positive"); @@ -75,15 +101,34 @@ public void testMetrics() { Assert.assertTrue( reporter.query(".total--success_request_ratio.RatioStat").value() > 0, "success_request_ratio should be positive"); + Assert.assertTrue( + reporter.query(".store_foo--request_key_size.Avg").value() > 0, + "Avg value for request key size should always be recorded"); + Assert.assertTrue( + reporter.query(".store_foo--request_key_size.Max").value() > 0, + "Max value for request key size should always be recorded"); - String[] percentileStrings = new String[] { "0_01", "0_01", "0_1", "1", "2", "3", "4", "5", "10", "20", "30", "40", - "50", "60", "70", "80", "90", "95", "99", "99_9" }; + Assert.assertTrue( + reporterForKVProfiling.query(".store_foo--request_key_size.Avg").value() > 0, + "Avg value for request key size should always be recorded"); + Assert.assertTrue( + reporterForKVProfiling.query(".store_foo--request_key_size.Max").value() > 0, + "Max value for request key size should always be recorded"); + + String[] fineGrainedPercentiles = new String[] { "0_01", "0_01", "0_1", "1", "2", "3", "4", "5", "10", "20", "30", + "40", "50", "60", "70", "80", "90", "95", "99", "99_9" }; + for (String fineGrainedPercentile: fineGrainedPercentiles) { + Assert.assertNull( + metricsRepository.getMetric(".store_foo--request_key_size." + fineGrainedPercentile + "thPercentile")); + Assert.assertNull( + metricsRepository.getMetric(".store_foo--request_value_size." + fineGrainedPercentile + "thPercentile")); - for (int i = 0; i < percentileStrings.length; i++) { Assert.assertTrue( - reporter.query(".store_foo--request_key_size." + percentileStrings[i] + "thPercentile").value() > 0); + reporterForKVProfiling.query(".store_foo--request_key_size." + fineGrainedPercentile + "thPercentile") + .value() > 0); Assert.assertTrue( - reporter.query(".store_foo--request_value_size." + percentileStrings[i] + "thPercentile").value() > 0); + reporterForKVProfiling.query(".store_foo--request_value_size." + fineGrainedPercentile + "thPercentile") + .value() > 0); } singleGetStats.handleStoreDeleted(STORE_FOO);