fix(metrics): do not drop the whole CloudWatch batch on one unmappable metric name - #19476
Open
rangareddy wants to merge 2 commits into
Open
fix(metrics): do not drop the whole CloudWatch batch on one unmappable metric name#19476rangareddy wants to merge 2 commits into
rangareddy wants to merge 2 commits into
Conversation
…e 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.
hudi-agent
reviewed
Aug 3, 2026
hudi-agent
left a comment
hudi-agent
left a comment
Contributor
There was a problem hiding this comment.
test
cc @yihua
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19476 +/- ##
=========================================
Coverage 76.96% 76.97%
- Complexity 33850 33858 +8
=========================================
Files 2575 2575
Lines 143372 143382 +10
Branches 17572 17574 +2
=========================================
+ Hits 110349 110366 +17
+ Misses 24758 24752 -6
+ Partials 8265 8264 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
voonhous
reviewed
Aug 3, 2026
voonhous
left a comment
voonhous
left a comment
Member
There was a problem hiding this comment.
Reviewed the fix against master. The approach is right and the bug is real -- comments inline, ordered correctness first, then cleanliness nits marked optional.
…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 apache#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.
hudi-agent
reviewed
Aug 4, 2026
hudi-agent
left a comment
hudi-agent
left a comment
Contributor
There was a problem hiding this comment.
Thanks for working on this! This PR changes the CloudWatch reporter so that an unmappable metric name (no dot, or an empty leading segment) is skipped and logged once rather than throwing mid-batch, which previously caused ScheduledReporter to suppress the exception and drop every metric in the interval. I traced the report path (empty metric data correctly sends no request), confirmed the unmappable-name set is bounded by a fixed vocabulary of dotless metric constants and is thread-safe, and verified the new tests recreate the reporter per test so the logged-once assertion is not order-sensitive. No issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
Collaborator
github-actions
Bot
added
size:M
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe the issue this Pull Request addresses
Closes #12182 and #13051. Both report the same thing: CloudWatch metrics are enabled, the job succeeds, and
no metrics arrive at all, with this in the logs every interval:
stageMetricDatumtakes the CloudWatchTabledimension from the part of the metric name before the firstdot, so a name with no dot cannot be mapped. HUDI-9068 (#12873) turned that
ArrayIndexOutOfBoundsExceptioninto an
IllegalArgumentException, but it still throws, and the throw is what produces the reported symptom.report()stages every gauge, counter, histogram, meter and timer into one list before callingputMetricData, so throwing part-way through means the request is never sent: one unmappable name costsevery metric in that interval.
ScheduledReporterthen suppresses the exception, so the user sees a log lineand an empty dashboard.
Dotless names still reach the reporter on current master.
HoodieMetadataMetrics#setMetriccallsmetrics.registerGauge(action, value)with no prefix at all (HoodieMetadataMetrics.java:176), unlikeMetrics#registerGauges, which applieshoodie.metrics.reporter.metricsname.prefix. Two that are registeredon normal paths:
lookup_meta_index_bloom_filters_file_count--BaseTableMetadata.java:212, on the bloom-index read path<partition>_bootstrap_error--HoodieBackedTableMetadataWriter.java:482There is a second way in, which review turned up. The prefix defaults to
""andMetrics#registerGaugesstill joins it with a dot (
Metrics.java:157), so a name can arrive as.foo. That splits into two parts andpassed the old length check, then asked CloudWatch for an empty
Tabledimension value. CloudWatch rejectsthe whole
PutMetricDatarequest for that, so the batch is lost again by a different route.So any table with the metadata table enabled and
CLOUDWATCHselected loses all of its metrics.Summary and Changelog
stageMetricDatumskips a metric it cannot map instead of throwing, so the rest of the batch is stillpublished. Both a name with no dot and a name whose first segment is empty are skipped.
offender does not spam the log every reporting cycle.
This replaces the
checkArgumentadded by #12873, so to be explicit about why: the goal there -- do notreport a metric under a wrong or missing table dimension -- is preserved, because the metric is still not
reported and is now named in a warning. What changes is that it no longer takes the other metrics down with
it. Fail-fast was not reachable here in any case, since
ScheduledReportercatches and suppresses everythingreport()throws, so the throw could never surface to a caller -- it could only delete the batch. Thesubstantive half of #12873, the
TABLE_SERVICE_EXECUTION_*prefixing inHoodieBackedTableMetadataWriter,is untouched.
Verification
testReportOnMetricsWithoutTableName, which asserted the throw, is replaced by four tests inTestCloudWatchReporter:testReportSkipsMetricsWithoutTableNameAndPublishesTheRest-- a batch holdinglookup_meta_index_bloom_filters_file_countplus a well-formed gauge publishes exactly one datum,testPrefix.gauge2, carryingTable=testTable.testReportSkipsMetricsWithAnEmptyTableName-- the same for.gauge1.testReportSendsNothingWhenEveryMetricIsUnmappable-- an interval with nothing mappable never callsputMetricData, rather than sending an empty request that AWS would reject.testUnmappableMetricIsLoggedOncePerName-- tworeport()calls over the same registry produce exactly oneWARN, captured with an
AbstractAppenderon theCloudWatchReporterlogger.Each one fails with its corresponding production change reverted, so none of them passes vacuously.
checkstyle:checkandapache-rat:checkare clean, and the rest ofhudi-awsis unaffected.Why the fix is reporter-side
Both prior fixes for this symptom went producer-side, so it is worth saying why this one does not.
1a5a9f7f03ec(HUDI-4439, #6164) fixed it by pushing the table name into the metadata table'sHoodieMetricsConfig.100e9ac47590(HUDI-9068, #12873) fixed two producer names and added thecheckArgumentthis PR replaces -- so that guard was a detector for producer bugs, not the fix.The producer fix is the right one and is filed as #19507.
HoodieMetadataMetricsalready receives aHoodieMetricsConfig(HoodieMetadataMetrics.java:91) and discards it, so retaining the prefix would fixboth the dotless names and the wrong-
Tablenames described under Impact, for every reporter at once. Itrenames metrics, which breaks existing Graphite, Prometheus, JMX and Datadog dashboards and needs a release
note, so it is not bundled here. This PR is explicitly the stop-the-bleeding half.
This is also the only place in the codebase where a reporter derives a dimension by splitting the metric
name, so the guard does not set a precedent other reporters have to follow. The closest analogue is
MetricUtils, used byDatadogReporterandPushGatewayReporter, and its convention is the one adoptedhere: missing optional structure degrades gracefully, while genuinely malformed input throws. "No dot" is
missing structure, not malformed input.
Impact
CloudWatch users whose tables register an unmappable metric name go from receiving no metrics to receiving
the well-formed ones.
They also start receiving metrics under a wrong
Tabledimension, and being billed for them. Names thatcontain a dot but no table pass the guard and are reported with the action as the table:
<action>.countand<action>.totalDurationgiveTable="initialize"andTable="lookup_partitions", while themetadata-partition stats give
Table="files"andTable="column_stats". Roughly 18 such metrics exist. Thiswas 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. #19507 is the producer-side fix; this PR deliberately does not rename metrics.
No effect on any other reporter, no effect on a CloudWatch setup that never produces such a name, and no API,
config or table format change.
Risk Level
low. One branch in one reporter, scoped to a case that currently throws. Verified that the well-formed
metrics still publish with the correct dimensions and that the rest of
hudi-awsis unaffected.Documentation Update
none
Contributor's checklist