rangareddy commented on code in PR #19476:
URL: https://github.com/apache/hudi/pull/19476#discussion_r3710140214
##########
hudi-aws/src/test/java/org/apache/hudi/aws/metrics/cloudwatch/TestCloudWatchReporter.java:
##########
@@ -168,18 +167,33 @@ public void testReporter() {
Mockito.verify(cloudWatchAsync).close();
}
+ /**
+ * A metric name with no dot has no table name to report under. Such names
do reach the reporter -
+ * {@code HoodieMetadataMetrics#setMetric} registers gauges without a
prefix, so
+ * {@code HoodieMetadataMetrics.getStats} contributes a bare {@code
partitionCount} - and this used to
+ * throw. {@link com.codahale.metrics.ScheduledReporter} suppresses whatever
{@code report()} throws, so
+ * the result was that 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.
+ */
@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("partitionCount", unmappable);
Review Comment:
You are right, and I verified it: `STAT_COUNT_PARTITION` is added only
inside `if (detailed)` at `HoodieMetadataMetrics.java:143-144`, while
`updateSizeMetrics` — the gauge-registering path — calls `getStats(false, ...)`
at :163. `grep -rn "\.stats()"` returns exactly one caller,
`MetadataCommand.java:197`, which prints the map. So `partitionCount` never
reaches a reporter and my headline example was unreachable.
Fixed everywhere it appeared: the fixture now uses
`lookup_meta_index_bloom_filters_file_count`, the javadoc cites
`BaseTableMetadata#getBloomFilters` instead of `getStats`, and the PR
description names that plus `<partition>_bootstrap_error` — with an explicit
note that the earlier revision cited `partitionCount` and why that was wrong,
so anyone reading the thread is not left with the bad example.
This was the most valuable comment on the PR: a committer checking the cited
example would have concluded the bug did not exist.
##########
hudi-aws/src/main/java/org/apache/hudi/aws/metrics/cloudwatch/CloudWatchReporter.java:
##########
@@ -276,8 +279,18 @@ private void stageMetricDatum(String metricName,
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) {
+ // The table dimension comes from the part before the first dot, so a
name without one cannot be
+ // mapped. 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: it contains no
dot, so there is no table name "
+ + "to report it under. Metrics are expected to be named
<table>.<metric>. Other metrics in this "
+ + "batch are unaffected, and this is logged once per metric
name.", metricName);
+ }
+ return;
+ }
String tableName = metricNameParts[0];
Review Comment:
Agreed, and it is now in Impact rather than buried in "Not in this PR":
> **They also start receiving metrics under a wrong `Table` dimension, and
being billed for them.** ... `<action>.count` and `<action>.totalDuration` give
`Table="initialize"`, `Table="lookup_partitions"`; the metadata-partition stats
give `Table="files"`, `Table="column_stats"`. Roughly 18 such metrics exist.
This was already broken, but invisible to exactly these users because the batch
died before anything was sent. CloudWatch bills per unique metric name plus
dimension set, so those become billable custom metrics the moment this merges.
The billing point is the part I had not considered at all — "already broken"
is true but irrelevant to these users, since for them it was latent until this
PR makes the batch send.
##########
hudi-aws/src/main/java/org/apache/hudi/aws/metrics/cloudwatch/CloudWatchReporter.java:
##########
@@ -276,8 +279,18 @@ private void stageMetricDatum(String metricName,
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) {
+ // The table dimension comes from the part before the first dot, so a
name without one cannot be
+ // mapped. 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: it contains no
dot, so there is no table name "
+ + "to report it under. Metrics are expected to be named
<table>.<metric>. Other metrics in this "
+ + "batch are unaffected, and this is logged once per metric
name.", metricName);
+ }
+ return;
Review Comment:
Filed as **#19507** and referenced from the code comment, the new tests and
the PR description. Thank you for the two precedents — I did not know about
`1a5a9f7f03ec`, and your reading of `100e9ac47590` is sharper than mine: the
`checkArgument` was a *detector* for producer bugs rather than the fix, which
is a much fairer way to describe what this PR replaces.
The description now has a section saying exactly that this is the
stop-the-bleeding half, naming both prior producer-side fixes, and stating that
`HoodieMetadataMetrics` already receives a `HoodieMetricsConfig` and discards
it — so the central fix is small, and the only reason it is not here is the
dashboard-renaming cost.
##########
hudi-aws/src/main/java/org/apache/hudi/aws/metrics/cloudwatch/CloudWatchReporter.java:
##########
@@ -276,8 +279,18 @@ private void stageMetricDatum(String metricName,
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) {
Review Comment:
Verified and fixed — this was a real hole in the guard.
`METRICS_REPORTER_PREFIX` defaults to `""` and its infer function only fires
when `hoodie.table.name` is in the props, and `Metrics#registerGauges` builds
`prefix + "." + k` from an `Option.of("")`, so `".foo"` is genuinely reachable.
```java
if (metricNameParts.length < 2 ||
StringUtils.isNullOrEmpty(metricNameParts[0])) {
```
`testReportSkipsMetricsWithAnEmptyTableName` covers `".gauge1"` alongside a
well-formed metric. Reverting just this condition fails it, so it is not
passing vacuously.
Your point that it "reproduces the same bug class this PR exists to fix" is
what made it a must-fix rather than a nice-to-have — an empty dimension value
fails the whole `PutMetricData`, so the batch would still be lost.
##########
hudi-aws/src/main/java/org/apache/hudi/aws/metrics/cloudwatch/CloudWatchReporter.java:
##########
@@ -66,6 +67,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();
Review Comment:
Added. `testUnmappableMetricIsLoggedOncePerName` calls `report()` twice
against the same registry and asserts exactly one WARN mentioning the metric
name, using a `CapturingAppender extends AbstractAppender` attached to the
`CloudWatchReporter` logger — the idiom you pointed at in
`TestPriorityBasedFileSystemView`.
You were right that `log4j-core` is available: it resolves at `provided`
scope for hudi-aws (`log4j-core-2.25.4.jar` is on the test classpath), so no
dependency change was needed.
Checked it discriminates: replacing `if
(unmappableMetricNames.add(metricName))` with `if (true)` fails the test. That
is the first of the two regressions you named; the second (suppressing the
datum rather than the log) is covered by the two tests that assert the
well-formed metric still publishes.
##########
hudi-aws/src/test/java/org/apache/hudi/aws/metrics/cloudwatch/TestCloudWatchReporter.java:
##########
@@ -168,18 +167,33 @@ public void testReporter() {
Mockito.verify(cloudWatchAsync).close();
}
+ /**
+ * A metric name with no dot has no table name to report under. Such names
do reach the reporter -
+ * {@code HoodieMetadataMetrics#setMetric} registers gauges without a
prefix, so
+ * {@code HoodieMetadataMetrics.getStats} contributes a bare {@code
partitionCount} - and this used to
+ * throw. {@link com.codahale.metrics.ScheduledReporter} suppresses whatever
{@code report()} throws, so
+ * the result was that 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.
+ */
@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("partitionCount", 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();
+
+ Mockito.verify(cloudWatchAsync,
Mockito.times(1)).putMetricData(putMetricDataRequestCaptor.capture());
+ List<MetricDatum> metricData =
putMetricDataRequestCaptor.getValue().metricData();
+ assertEquals(1, metricData.size(),
Review Comment:
Added — `testReportSendsNothingWhenEveryMetricIsUnmappable`, with a registry
of only unmappable gauges and
```java
Mockito.verify(cloudWatchAsync,
Mockito.never()).putMetricData(ArgumentMatchers.any(PutMetricDataRequest.class));
```
You were right about the strictness too: `cloudWatchAsync` is `@Mock(lenient
= true)`, so the unused `putMetricData` stub from `setup()` does not trip
`UnnecessaryStubbing`.
Agreed the path is already correct and this pins it rather than fixing
anything — an empty `PutMetricDataRequest` would be rejected by AWS, and
nothing stopped a refactor from sending one.
##########
hudi-aws/src/main/java/org/apache/hudi/aws/metrics/cloudwatch/CloudWatchReporter.java:
##########
@@ -276,8 +279,18 @@ private void stageMetricDatum(String metricName,
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) {
+ // The table dimension comes from the part before the first dot, so a
name without one cannot be
+ // mapped. 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: it contains no
dot, so there is no table name "
+ + "to report it under. Metrics are expected to be named
<table>.<metric>. Other metrics in this "
+ + "batch are unaffected, and this is logged once per metric
name.", metricName);
Review Comment:
Applied, with #19507 as the referenced issue:
```java
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);
```
You are right that the old wording asserted a convention Hudi does not
follow — no metadata metric carries a table name — and that it gave an operator
nothing to act on. Naming the config at least tells them where the prefix
normally comes from, and the issue link says why these names are exempt.
##########
hudi-aws/src/test/java/org/apache/hudi/aws/metrics/cloudwatch/TestCloudWatchReporter.java:
##########
@@ -168,18 +167,33 @@ public void testReporter() {
Mockito.verify(cloudWatchAsync).close();
}
+ /**
+ * A metric name with no dot has no table name to report under. Such names
do reach the reporter -
+ * {@code HoodieMetadataMetrics#setMetric} registers gauges without a
prefix, so
+ * {@code HoodieMetadataMetrics.getStats} contributes a bare {@code
partitionCount} - and this used to
+ * throw. {@link com.codahale.metrics.ScheduledReporter} suppresses whatever
{@code report()} throws, so
+ * the result was that 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.
+ */
@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("partitionCount", 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();
+
+ 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);
reporter.stop();
Mockito.verify(cloudWatchAsync).close();
Review Comment:
Half-taken. I did not repeat the `stop()` + `verify(close())` tail in any of
the four new tests, so the re-pasting you were guarding against is gone.
I left the existing one in `testReporter` rather than moving it to
`@AfterEach`, because that would apply the close assertion to every test in the
class including future ones, and it is an assertion about stop/close plumbing
rather than about whatever each test covers. Only one occurrence remains, so
the smell is already resolved. Happy to move it if you would rather have it
uniform.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
