feat: support customized metrics reporter#18710
feat: support customized metrics reporter#18710HuangZhenQiu wants to merge 1 commit intoapache:masterfrom
Conversation
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.
Thanks for the contribution! This PR adds a CUSTOM MetricsReporterType and refactors MetricsReporterFactory to dispatch custom reporter loading via the switch statement, with support for multiple constructor signatures. The main thing worth a closer look is backward compatibility for users who already configure hoodie.metrics.reporter.class without reporter.type — see inline comments. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here. One minor readability nit on the guard-clause pattern; everything else looks clean.
| case SLF4J: | ||
| reporter = new Slf4jMetricsReporter(registry); | ||
| break; | ||
| case CUSTOM: |
There was a problem hiding this comment.
🤖 This looks like a silent backward-incompatible change. Before this PR, if a user set hoodie.metrics.reporter.class=com.foo.MyReporter without touching reporter.type, the class-name check ran first and the custom reporter was loaded. After this PR, since METRICS_REPORTER_TYPE_VALUE defaults to GRAPHITE, the same config will silently fall into the GRAPHITE case and ignore the user's custom class — they'll get a Graphite reporter (or a startup failure) with no warning. Could you preserve the prior behavior — e.g., if reporter.class is set and reporter.type is left at the default, treat it as CUSTOM (or at least log a clear warning) so existing deployments aren't silently broken? @yihua
- AI-generated; verify before applying. React 👍/👎 to flag quality.
| .withDocumentation("Fully qualified class name of a custom MetricsReporter implementation. " | ||
| + "The class must extend MetricsReporter and support one of these constructors (tried in order): " | ||
| + "(Properties, MetricRegistry), (HoodieMetricsConfig, MetricRegistry), or no-arg. " | ||
| + "Set reporter.type=CUSTOM or leave it unset when using this option."); |
There was a problem hiding this comment.
🤖 The phrase "or leave it unset when using this option" doesn't match the actual code behavior. METRICS_REPORTER_TYPE_VALUE defaults to GRAPHITE (not unset), and even when it is null createReporter returns Option.empty() — neither path will load the custom class. Users effectively must set reporter.type=CUSTOM. Could you tighten this doc accordingly?
- AI-generated; verify before applying. React 👍/👎 to flag quality.
| break; | ||
| case CUSTOM: | ||
| String reporterClassName = metricsConfig.getMetricReporterClassName(); | ||
| if (!StringUtils.isNullOrEmpty(reporterClassName)) { |
There was a problem hiding this comment.
🤖 nit: could you invert this into a guard clause? Throwing first and then falling through to the happy-path return removes the else branch and makes the intent clearer — e.g. if (StringUtils.isNullOrEmpty(reporterClassName)) { throw …; } return Option.of(loadCustomReporter(…));
- AI-generated; verify before applying. React 👍/👎 to flag quality.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18710 +/- ##
=========================================
Coverage 68.14% 68.15%
- Complexity 29077 29093 +16
=========================================
Files 2522 2522
Lines 141177 141184 +7
Branches 17514 17512 -2
=========================================
+ Hits 96208 96225 +17
+ Misses 37061 37051 -10
Partials 7908 7908
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
This change is not needed. |
4 participants
Describe the issue this Pull Request addresses
Add Custom reporter type for Hudi users to customize any metrics reporter for internal infra requirement
Summary and Changelog
Impact
none
Risk Level
none
Documentation Update
none
Contributor's checklist