zentol commented on a change in pull request #8002: [FLINK-11923][metrics]
MetricRegistryConfiguration provides MetricReporters Suppliers
URL: https://github.com/apache/flink/pull/8002#discussion_r274009874
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/metrics/MetricRegistryImpl.java
##########
@@ -113,19 +114,12 @@ public MetricRegistryImpl(MetricRegistryConfiguration
config) {
// by default, don't report anything
LOG.info("No metrics reporter configured, no metrics
will be exposed/reported.");
} else {
- // we have some reporters so
- for (Tuple2<String, Configuration>
reporterConfiguration: reporterConfigurations) {
- String namedReporter = reporterConfiguration.f0;
- Configuration reporterConfig =
reporterConfiguration.f1;
-
- final String className =
reporterConfig.getString(ConfigConstants.METRICS_REPORTER_CLASS_SUFFIX, null);
- if (className == null) {
- LOG.error("No reporter class set for
reporter " + namedReporter + ". Metrics might not be exposed/reported.");
- continue;
- }
+ for (ReporterSetup reporterSetup :
reporterConfigurations) {
Review comment:
I assumed you meant that the reporter class would have to hard-code the
name, but you're suggesting that the reporter parses the interval and name from
the configuration I guess.
~~So for one this wouldn't really work with the subsequent introduction of
factories, which requires the MetricConfig to be assembled before the reporter
is even instantiated. One could move the logic into the factory of course; my
point is that this is not something we can do _now_.~
Reporters (or factories for that matter) shouldn't be aware that reporter
names actually exist; this is an implementation detail of the metric system to
allow for distinct configurations. Whether multiple reporters exist or not is
not irrelevant to a given reporter, so they shouldn't have to deal with it. I
also don't see a way how a reporter would be able to determine it's own name
from the given configuration. The metric-wide configuration does not contain
this info in an obvious fashion, and the metric config does not contain it at
all. I can only see this working if we (the MetricRegistry/-Configuration)
explicitly write it into the MetricConfig, but this obviously doesn't make any
sense.
Admittedly, the interval makes more sense. It is true that the current
reporter configuration is a mix between reporter-facing options (like
reporter-specific arguments) and system-facing options (like the interval). The
current approach however allows us to ensure that certain configuration options
exist and are actually applied; in a design where the `Scheduled` reporter
provides the interval you cannot guarantee that the interval is configurable
for users, or that a configured interval is respected.
The same applies to other system-facing reporter options, like delimiters.
So long as reporter don't massively go out of their way to avoid it (i.e. not
working at all against the MetricGroup interface (with some currently existing
exceptions)) the configured delimiter _will_ be used.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
