voonhous commented on code in PR #19476:
URL: https://github.com/apache/hudi/pull/19476#discussion_r3703934066
##########
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:
The PR's headline example is not reachable: `partitionCount` can never reach
the reporter, so this fixture does not represent the reported bug.
`STAT_COUNT_PARTITION` is only put into the stats map when `detailed ==
true` (`HoodieMetadataMetrics.java:143-144`), but the gauge-registering path
calls `getStats(false, ...)` (`HoodieMetadataMetrics.java:163`). The only
`detailed=true` caller is `HoodieBackedTableMetadata.stats()`, whose sole
consumer in the tree is `hudi-cli/.../MetadataCommand.java:197`, which prints
the map and never registers a gauge:
```
grep -rn '\.stats()' --include='*.java' . | grep -v /target/
```
The bug is real, just not via this name. Two dotless names that genuinely
are registered on master:
- `lookup_meta_index_bloom_filters_file_count` --
`BaseTableMetadata.java:212`, on the normal bloom-index read path
- `<partition>_bootstrap_error` -- `HoodieBackedTableMetadataWriter.java:482`
Please use a real one here:
```suggestion
gauges.put("lookup_meta_index_bloom_filters_file_count", unmappable);
```
Then update the javadoc above (lines 171-176) to cite
`BaseTableMetadata#getBloomFilters` instead of
`HoodieMetadataMetrics.getStats`, and fix the same claim in the PR description
and in its Verification transcript. As written, a committer who checks the
cited example concludes the bug does 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:
The Impact section overstates the result. Users do not go from "no metrics"
to "every well-formed metric" -- they also start receiving metrics under a
wrong `Table` dimension.
Names that contain a dot but no table pass the guard above and land here
with the *action* as `tableName`:
- `<action>.count` / `<action>.totalDuration` --
`HoodieMetadataMetrics.java:156-157` -> `Table="lookup_partitions"`,
`Table="initialize"`, ...
-
`<mdt_partition>.{baseFileCount,logFileCount,totalBaseFileSizeInBytes,totalLogFileSizeInBytes}`
-- `HoodieMetadataMetrics.java:137-140` -> `Table="files"`,
`Table="column_stats"`, ...
These really are registered: `TestJavaHoodieBackedMetadata.java:2612-2618`
asserts the registry contains `initialize.count`, `files.baseFileCount` and
friends.
This was already broken before your change, but for exactly the affected
users it was invisible, because the batch died before anything could be sent.
CloudWatch bills per unique metric name + dimension set, so roughly 18+ bogus
custom metrics start being charged the moment this merges.
Please state this plainly in the Impact section rather than only in "Not in
this PR", so a committer can weigh the cost implication.
##########
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:
The guard misses the empty-table-name case, which reproduces the same bug
class this PR exists to fix.
`hoodie.metrics.reporter.metricsname.prefix` defaults to `""`
(`HoodieMetricsConfig.java:78-81`; the infer-from-`hoodie.table.name` at :82-87
only fires when that key is present in the props). With an empty prefix,
`Metrics.java:157` builds `"" + "." + k`, i.e. a leading dot. `".foo"` then
splits to `["", "foo"]`, passes `length >= 2`, and emits `Dimension(Table,
"")`. CloudWatch requires a dimension value of at least one non-whitespace
character and rejects the whole `PutMetricData` request with
`InvalidParameterValue`, so one such metric again takes down up to
`maxDatumsPerRequest` datums with it.
One line closes it:
```suggestion
if (metricNameParts.length < 2 ||
StringUtils.isNullOrEmpty(metricNameParts[0])) {
```
Needs `import org.apache.hudi.common.util.StringUtils;`. Please also add a
`".foo"` case to the new test.
##########
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:
Worth flagging the direction explicitly here, because both prior fixes for
this exact symptom went producer-side and a committer will likely ask why this
one does not:
- `1a5a9f7f03ec` [HUDI-4439] "Fix Amazon CloudWatch reporter for metadata
enabled tables" (#6164) fixed it by pushing `HoodieWriteConfig.TBL_NAME` into
the metadata table's `HoodieMetricsConfig`. Zero reporter changes.
- `100e9ac47590` [HUDI-9068] (#12873) fixed two producer names *and* added
the `checkArgument` you are removing -- i.e. that guard was a detector for
producer bugs, not the fix itself.
The producer fix is also small and central: `HoodieMetadataMetrics` already
receives `HoodieMetricsConfig` at `HoodieMetadataMetrics.java:91` and discards
it. Retaining `getMetricReporterMetricsNamePrefix()` and applying it in
`setMetric`/`incrementMetric` would fix both the dotless case and the
wrong-`Table` case, for every reporter at once.
Your counter-argument (renaming breaks existing
Graphite/Prometheus/JMX/Datadog dashboards) is legitimate and I am not asking
you to do it in this PR. But please file the follow-up JIRA now and reference
its id from this code comment and from the PR description, so this lands as
explicitly the "stop the bleeding" half rather than reading as the whole fix.
##########
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);
Review Comment:
Supportive note rather than a request, since the obvious objection to this
PR is "you reverted HUDI-9068": there is house precedent on your side, and
citing it in the description would help this land.
This is the only place in the codebase where a reporter derives a dimension
by splitting the metric name:
```
grep -rn "split(" hudi-common/src/main/java/org/apache/hudi/metrics
hudi-aws/src/main/java
```
The closest analogue is `MetricUtils`, used by `DatadogReporter.java:96` and
`PushGatewayReporter.java:133`. Its convention is exactly the one you are
adopting: missing optional structure degrades gracefully (`return
Pair.of(tokens[0], "")`, `MetricUtils.java:61`), while genuinely malformed
input throws (`:55`). "No dot" is missing structure, not malformed input.
Also worth stating explicitly in the description: #12873's substantive half
is untouched here. The `TABLE_SERVICE_EXECUTION_*` prefixing survives at
`HoodieBackedTableMetadataWriter.java:1565-1579` and its assertions at
`TestJavaHoodieBackedMetadata.java:2622-2623`. Only the reporter-side throw
changes.
(Unrelated, for a future PR: `MetricUtils.getMetricAndLabels` throws from
inside `DatadogReporter.report()` and `PushGatewayReporter.report()` on a name
containing two `;` -- identical whole-batch-loss bug class, still live on
master.)
##########
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:
This field is the only new mutable state in the class, and it has no test
coverage: its "already seen" branch is never taken, because the new test calls
`report()` exactly once. This is precisely the 1 partial line codecov reports
(80% patch coverage).
Concretely, two regressions pass the suite unchanged today: deleting this
field entirely and logging unconditionally, and a change where the set
suppressed the *datum* rather than the *log*.
The repo already has the idiom -- `TestLogAppender extends AbstractAppender`
at `TestPriorityBasedFileSystemView.java:877-892`, used at `:139-160`.
`log4j-core` reaches hudi-aws at test scope through `hudi-tests-common`, and
`org.apache.hudi` logs at DEBUG/WARN under both surefire log4j2 configs, so a
WARN is captured either way.
Please add a test that calls `reporter.report()` twice against the same
registry and asserts exactly one WARN event from `CloudWatchReporter`.
##########
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:
nit, feel free to ignore: the message states a convention Hudi does not
actually follow, and is not actionable for an operator.
"Metrics are expected to be named `<table>.<metric>`" is false for every
metadata metric -- `HoodieMetadataMetrics.java:137-140` and `:156-157` produce
`<mdt_partition>.<stat>` and `<action>.count`, none of which carry a table
name. And the operator has no knob that fixes the names being skipped, so the
warning reports a problem without pointing anywhere.
```suggestion
log.warn("Not reporting metric \"{}\" to CloudWatch: it has no dot,
so no table name can be "
+ "derived for the Table dimension. Metric names normally carry
the "
+ "hoodie.metrics.reporter.metricsname.prefix, but some
Hudi-internal metadata metrics do "
+ "not (see HUDI-XXXX). Other metrics in this batch are
unaffected, and this is logged "
+ "once per metric name.", metricName);
```
Fill in the follow-up JIRA id from the other comment.
##########
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:
nit, feel free to ignore: `reporter.stop();` +
`Mockito.verify(cloudWatchAsync).close();` is copied verbatim from
`testReporter` (lines 166-167). It asserts stop/close plumbing unrelated to
this fix, and a failure there would mask the real assertion above it.
Consider moving it into an `@AfterEach` so the tail does not get re-pasted
into every future test in this class.
##########
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:
This misses the one genuinely new runtime state the change introduces: an
interval in which *every* metric is unmappable, leaving `metricsData` empty.
That path is currently correct -- the partition loop at
`CloudWatchReporter.java:220-223` builds zero partitions from an empty list, so
`putMetricData` is never called -- but nothing asserts it. A later refactor
that sent an empty `PutMetricDataRequest` (AWS rejects it with a validation
error) would pass the suite unchanged.
Please add a case whose registry holds only dotless gauges, and assert:
```java
Mockito.verify(cloudWatchAsync,
Mockito.never()).putMetricData(ArgumentMatchers.any(PutMetricDataRequest.class));
```
That is safe under `MockitoExtension` STRICT_STUBS because `cloudWatchAsync`
is `@Mock(lenient = true)`, so the unused `putMetricData` stub in `setup()`
will not trip `UnnecessaryStubbing`.
--
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]
