This is an automated email from the ASF dual-hosted git repository.
voonhous pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/hudi.git
The following commit(s) were added to refs/heads/master by this push:
new c63c9bfa79fd fix(metrics): do not drop the whole CloudWatch batch on
one unmappable metric name (#19476)
c63c9bfa79fd is described below
commit c63c9bfa79fdb5a606fd7452fdfb3510f2f5739a
Author: Ranga Reddy <[email protected]>
AuthorDate: Tue Aug 4 21:06:57 2026 +0530
fix(metrics): do not drop the whole CloudWatch batch on one unmappable
metric name (#19476)
* fix(metrics): do not drop the whole CloudWatch batch on one unmappable
metric name
stageMetricDatum derives the CloudWatch Table dimension from the part of
the metric
name before the first dot, so a name without one cannot be mapped. It threw
for
that case, and report() stages every metric into one list before calling
putMetricData, so throwing part-way through meant the request was never
sent: one
unmappable name cost every metric in the interval. ScheduledReporter then
suppresses the exception, so the user saw a log line and an empty dashboard.
Such names still reach the reporter on master.
HoodieMetadataMetrics#setMetric
registers gauges with no prefix, unlike Metrics#registerGauges, so getStats
contributes a bare partitionCount and BaseTableMetadata a bare
lookup_meta_index_bloom_filters_file_count.
Skip the metric that cannot be mapped and publish the rest, logging the
name once
rather than every interval. The intent of the previous check is kept - the
metric is
still not reported under a wrong table, and is now named in a warning -
without
taking the other metrics down with it. Fail-fast was never reachable here
anyway,
since ScheduledReporter suppresses whatever report() throws.
* fix(metrics): also skip metrics whose table name is empty, and cover the
log-once path
Review feedback.
The headline example in this PR was unreachable and I have replaced it.
partitionCount
comes from HoodieMetadataMetrics.getStats only when detailed == true, while
the
gauge-registering path calls getStats(false, ...); the only detailed=true
caller is
HoodieBackedTableMetadata.stats(), whose sole consumer prints the map in
hudi-cli and
registers nothing. The fixture and javadoc now use
lookup_meta_index_bloom_filters_file_count, which BaseTableMetadata
registers on the
normal bloom-index read path.
An empty first segment was still losing the batch, which is the same bug
class this PR
exists to fix. hoodie.metrics.reporter.metricsname.prefix defaults to "" and
Metrics#registerGauges still joins it with a dot, so ".foo" splits into two
parts, passed
the length check, and asked CloudWatch for an empty Table dimension value -
which it
rejects for the entire PutMetricData request. The guard now also rejects an
empty first
segment.
The warning no longer claims a <table>.<metric> convention that Hudi does
not follow: no
metadata metric carries a table name, and an operator has no knob that
changes the names
being skipped. It now names the prefix config and points at #19507 for the
producer side.
Three tests added: the empty-table-name case, an interval where every
metric is
unmappable asserting that no empty PutMetricData request is sent, and one
that reports
twice and asserts a single WARN, so the once-per-name set is no longer
uncovered. Both new
guards fail the suite when reverted.
---
.../aws/metrics/cloudwatch/CloudWatchReporter.java | 26 +++-
.../metrics/cloudwatch/TestCloudWatchReporter.java | 145 +++++++++++++++++++--
2 files changed, 158 insertions(+), 13 deletions(-)
diff --git
a/hudi-aws/src/main/java/org/apache/hudi/aws/metrics/cloudwatch/CloudWatchReporter.java
b/hudi-aws/src/main/java/org/apache/hudi/aws/metrics/cloudwatch/CloudWatchReporter.java
index ba9abc55bef7..470fda3dfa1f 100644
---
a/hudi-aws/src/main/java/org/apache/hudi/aws/metrics/cloudwatch/CloudWatchReporter.java
+++
b/hudi-aws/src/main/java/org/apache/hudi/aws/metrics/cloudwatch/CloudWatchReporter.java
@@ -20,7 +20,7 @@ package org.apache.hudi.aws.metrics.cloudwatch;
import org.apache.hudi.aws.credentials.HoodieAWSCredentialsProviderFactory;
import org.apache.hudi.common.util.Option;
-import org.apache.hudi.common.util.ValidationUtils;
+import org.apache.hudi.common.util.StringUtils;
import com.codahale.metrics.Clock;
import com.codahale.metrics.Counter;
@@ -45,7 +45,9 @@ import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Properties;
+import java.util.Set;
import java.util.SortedMap;
+import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
@@ -66,6 +68,8 @@ public class CloudWatchReporter extends ScheduledReporter {
private final String prefix;
private final String namespace;
private final int maxDatumsPerRequest;
+ /** Metric names already reported as unmappable, so the warning is logged
once rather than every interval. */
+ private final Set<String> unmappableMetricNames =
ConcurrentHashMap.newKeySet();
public static Builder forRegistry(MetricRegistry registry) {
return new Builder(registry);
@@ -276,8 +280,24 @@ public class CloudWatchReporter extends ScheduledReporter {
long timestampMilliSec,
List<MetricDatum> metricData) {
String[] metricNameParts = metricName.split("\\.", 2);
- ValidationUtils.checkArgument(metricNameParts.length >= 2,
- "metricName doesn't follow the naming convention and doesn't
contain a dot as splitter! metricName:" + metricName);
+ if (metricNameParts.length < 2 ||
StringUtils.isNullOrEmpty(metricNameParts[0])) {
+ // The table dimension comes from the part before the first dot, so a
name without one, or one whose
+ // first segment is empty, cannot be mapped. An empty first segment is
reachable:
+ // hoodie.metrics.reporter.metricsname.prefix defaults to "" and
Metrics#registerGauges still joins it
+ // with a dot, producing ".foo" - and CloudWatch rejects a whole
PutMetricData request whose dimension
+ // value is empty, which would lose the batch again.
+ //
+ // Skip just this metric rather than throwing: ScheduledReporter
suppresses whatever report() throws,
+ // so failing here dropped every metric staged in the same interval and
left no metrics in CloudWatch
+ // at all.
+ if (unmappableMetricNames.add(metricName)) {
+ log.warn("Not reporting metric \"{}\" to CloudWatch: no table name can
be derived for the Table "
+ + "dimension. Metric names normally carry
hoodie.metrics.reporter.metricsname.prefix, but some "
+ + "Hudi-internal metadata metrics do not (see HUDI issue #19507).
Other metrics in this batch "
+ + "are unaffected, and this is logged once per metric name.",
metricName);
+ }
+ return;
+ }
String tableName = metricNameParts[0];
metricData.add(MetricDatum.builder()
diff --git
a/hudi-aws/src/test/java/org/apache/hudi/aws/metrics/cloudwatch/TestCloudWatchReporter.java
b/hudi-aws/src/test/java/org/apache/hudi/aws/metrics/cloudwatch/TestCloudWatchReporter.java
index 0073f3687db2..d6fb7a4cfb7e 100644
---
a/hudi-aws/src/test/java/org/apache/hudi/aws/metrics/cloudwatch/TestCloudWatchReporter.java
+++
b/hudi-aws/src/test/java/org/apache/hudi/aws/metrics/cloudwatch/TestCloudWatchReporter.java
@@ -27,6 +27,12 @@ import com.codahale.metrics.Meter;
import com.codahale.metrics.MetricFilter;
import com.codahale.metrics.MetricRegistry;
import com.codahale.metrics.Timer;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.core.LogEvent;
+import org.apache.logging.log4j.core.LoggerContext;
+import org.apache.logging.log4j.core.appender.AbstractAppender;
+import org.apache.logging.log4j.core.config.LoggerConfig;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
@@ -43,6 +49,8 @@ import
software.amazon.awssdk.services.cloudwatch.model.MetricDatum;
import software.amazon.awssdk.services.cloudwatch.model.PutMetricDataRequest;
import software.amazon.awssdk.services.cloudwatch.model.PutMetricDataResponse;
+import java.util.ArrayList;
+import java.util.Collections;
import java.util.List;
import java.util.SortedMap;
import java.util.TreeMap;
@@ -54,7 +62,6 @@ import static
org.apache.hudi.aws.metrics.cloudwatch.CloudWatchReporter.DIMENSIO
import static
org.apache.hudi.aws.metrics.cloudwatch.CloudWatchReporter.DIMENSION_METRIC_TYPE_KEY;
import static
org.apache.hudi.aws.metrics.cloudwatch.CloudWatchReporter.DIMENSION_TABLE_NAME_KEY;
import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertThrows;
@ExtendWith(MockitoExtension.class)
public class TestCloudWatchReporter {
@@ -168,21 +175,139 @@ public class TestCloudWatchReporter {
Mockito.verify(cloudWatchAsync).close();
}
+ /**
+ * A metric name with no dot has no table name to report under, and such
names do reach the reporter:
+ * {@code HoodieMetadataMetrics#setMetric} registers gauges without the
metrics-name prefix, so
+ * {@code BaseTableMetadata#getBloomFilters} contributes a bare
+ * {@code lookup_meta_index_bloom_filters_file_count} on the normal
bloom-index read path. This used to
+ * throw, and {@link com.codahale.metrics.ScheduledReporter} suppresses
whatever {@code report()} throws,
+ * so no metrics reached CloudWatch at all - which is what #12182 and #13051
report. The unmappable metric
+ * is now skipped and the rest of the batch is still published. See HUDI
issue #19507 for the producer side.
+ */
@Test
- public void testReportOnMetricsWithoutTableName() {
+ public void testReportSkipsMetricsWithoutTableNameAndPublishesTheRest() {
SortedMap<String, Gauge> gauges = new TreeMap<>();
- Gauge<Long> gauge1 = () -> 100L;
- Gauge<Double> gauge2 = () -> 100.1;
- gauges.put("gauge1", gauge1);
- gauges.put(TABLE_NAME + ".gauge2", gauge2);
+ Gauge<Long> unmappable = () -> 7L;
+ Gauge<Double> wellFormed = () -> 100.1;
+ gauges.put("lookup_meta_index_bloom_filters_file_count", unmappable);
+ gauges.put(TABLE_NAME + ".gauge2", wellFormed);
Mockito.when(metricRegistry.getGauges(MetricFilter.ALL)).thenReturn(gauges);
- // should fail if metric name doesn't have at least two parts
- assertThrows(IllegalArgumentException.class, () -> reporter.report());
+ reporter.report();
- reporter.stop();
- Mockito.verify(cloudWatchAsync).close();
+ Mockito.verify(cloudWatchAsync,
Mockito.times(1)).putMetricData(putMetricDataRequestCaptor.capture());
+ List<MetricDatum> metricData =
putMetricDataRequestCaptor.getValue().metricData();
+ assertEquals(1, metricData.size(),
+ "The unmappable metric should be skipped and the well-formed one still
published");
+ assertEquals(PREFIX + ".gauge2", metricData.get(0).metricName());
+ assertEquals(wellFormed.getValue(), metricData.get(0).value());
+ assertDimensions(metricData.get(0).dimensions(),
DIMENSION_GAUGE_TYPE_VALUE);
+ }
+
+ /**
+ * An empty first segment is reachable: {@code
hoodie.metrics.reporter.metricsname.prefix} defaults to
+ * {@code ""} and {@code Metrics#registerGauges} still joins it with a dot,
giving {@code ".foo"}. That
+ * splits into two parts and so passed the length check, then asked
CloudWatch for an empty {@code Table}
+ * dimension value, which it rejects for the whole PutMetricData request -
losing the batch again.
+ */
+ @Test
+ public void testReportSkipsMetricsWithAnEmptyTableName() {
+ SortedMap<String, Gauge> gauges = new TreeMap<>();
+ gauges.put(".gauge1", (Gauge<Long>) () -> 7L);
+ gauges.put(TABLE_NAME + ".gauge2", (Gauge<Long>) () -> 100L);
+
+
Mockito.when(metricRegistry.getGauges(MetricFilter.ALL)).thenReturn(gauges);
+
+ reporter.report();
+
+ Mockito.verify(cloudWatchAsync,
Mockito.times(1)).putMetricData(putMetricDataRequestCaptor.capture());
+ List<MetricDatum> metricData =
putMetricDataRequestCaptor.getValue().metricData();
+ assertEquals(1, metricData.size(), "a metric whose table name is empty
should be skipped");
+ assertEquals(PREFIX + ".gauge2", metricData.get(0).metricName());
+ }
+
+ /**
+ * An interval in which every metric is unmappable leaves nothing staged.
CloudWatch rejects an empty
+ * PutMetricData request, so the reporter must not send one.
+ */
+ @Test
+ public void testReportSendsNothingWhenEveryMetricIsUnmappable() {
+ SortedMap<String, Gauge> gauges = new TreeMap<>();
+ gauges.put("lookup_meta_index_bloom_filters_file_count", (Gauge<Long>) ()
-> 7L);
+ gauges.put("bootstrap_error", (Gauge<Long>) () -> 1L);
+
+
Mockito.when(metricRegistry.getGauges(MetricFilter.ALL)).thenReturn(gauges);
+
+ reporter.report();
+
+ Mockito.verify(cloudWatchAsync,
Mockito.never()).putMetricData(ArgumentMatchers.any(PutMetricDataRequest.class));
+ }
+
+ /**
+ * The unmappable-name set exists so a persistent offender is logged once
rather than every reporting
+ * interval. Without this, deleting the set and logging unconditionally
would pass the suite.
+ */
+ @Test
+ public void testUnmappableMetricIsLoggedOncePerName() {
+ SortedMap<String, Gauge> gauges = new TreeMap<>();
+ gauges.put("lookup_meta_index_bloom_filters_file_count", (Gauge<Long>) ()
-> 7L);
+
Mockito.when(metricRegistry.getGauges(MetricFilter.ALL)).thenReturn(gauges);
+
+ CapturingAppender appender =
CapturingAppender.attachTo(CloudWatchReporter.class);
+ try {
+ reporter.report();
+ reporter.report();
+ } finally {
+ appender.detach();
+ }
+
+ assertEquals(1,
appender.warningsContaining("lookup_meta_index_bloom_filters_file_count"),
+ "a persistent unmappable name should be warned about once, not once
per interval");
+ }
+
+ /** Captures WARN events from a single logger, so "logged once" can be
asserted. */
+ private static final class CapturingAppender extends AbstractAppender {
+ private final List<String> warnings = Collections.synchronizedList(new
ArrayList<>());
+ private final LoggerConfig loggerConfig;
+ private final Level previousLevel;
+
+ private CapturingAppender(LoggerConfig loggerConfig) {
+ super("CapturingAppender", null, null, true, null);
+ this.loggerConfig = loggerConfig;
+ this.previousLevel = loggerConfig.getLevel();
+ }
+
+ static CapturingAppender attachTo(Class<?> loggerFor) {
+ LoggerContext context = (LoggerContext) LogManager.getContext(false);
+ LoggerConfig loggerConfig =
context.getConfiguration().getLoggerConfig(loggerFor.getName());
+ CapturingAppender appender = new CapturingAppender(loggerConfig);
+ appender.start();
+ loggerConfig.addAppender(appender, Level.WARN, null);
+ loggerConfig.setLevel(Level.WARN);
+ context.updateLoggers();
+ return appender;
+ }
+
+ void detach() {
+ loggerConfig.removeAppender(getName());
+ loggerConfig.setLevel(previousLevel);
+ ((LoggerContext) LogManager.getContext(false)).updateLoggers();
+ stop();
+ }
+
+ long warningsContaining(String needle) {
+ synchronized (warnings) {
+ return warnings.stream().filter(m -> m.contains(needle)).count();
+ }
+ }
+
+ @Override
+ public void append(LogEvent event) {
+ if (event.getLevel().isMoreSpecificThan(Level.WARN)) {
+ warnings.add(event.getMessage().getFormattedMessage());
+ }
+ }
}
private void assertDimensions(List<Dimension> actualDimensions, String
metricTypeDimensionVal) {
