Potential performance improvement in YammerPrometheusMetricsReporter

[mimaison]

Currently during collect(), we iterate through all the metrics from the Yammer registries to apply the allow list filter.

It's possible to register listeners onto a Yammer registry. We should check if we could use this pattern and apply the filtering as metrics are added or removed instead of in collect(). This is similar to what's described in https://github.com/strimzi/metrics-reporter/issues/9 for KafkaPrometheusMetricsReporter.

[OwenCorrigan76]

@mimaison This is my understanding of It's possible to register listeners onto a Yammer registry: The Yammer metrics library supports adding listeners that can react to changes or events related to the metrics being collected and managed by the registry. MetricsRegistry is a central place where metrics like counters, gauges etc. are registered, allowing you to manage and query the metrics efficiently. Listeners can be attached to the registry to listen for specific events such as a new metric, and the listener can handle the event. This can be done when a metric is added, removed, or updated rather than when it is collected, as the current logic is.

Is that a correct assumption are is there something I'm missing? Thanks

[OwenCorrigan76]

These are the relevant lines if the KafkaMetriceReporter?? https://github.com/strimzi/metrics-reporter/blob/main/src/main/java/io/strimzi/kafka/metrics/KafkaPrometheusMetricsReporter.java#L47C1-L59C6

[mimaison]

Is that a correct assumption are is there something I'm missing? Thanks

Yes that's a good summary.

[mimaison]

These are the relevant lines if the KafkaMetriceReporter?? https://github.com/strimzi/metrics-reporter/blob/main/src/main/java/io/strimzi/kafka/metrics/KafkaPrometheusMetricsReporter.java#L47C1-L59C6

This ticket is about YammerPrometheusMetricsReporter, so I'm not sure why you're mentioning KafkaPrometheusMetricsReporter. Why do you think these lines are relevant to this ticket?

[OwenCorrigan76]

You referenced this abaove This is similar to what's described in https://github.com/strimzi/metrics-reporter/issues/9 for KafkaPrometheusMetricsReporter. So I was looking at the changes in the KafkaPrometheusMetricsReporter to see what's different regarding registry

[OwenCorrigan76]

Do we want to: 1) Register listeners for metric additions and removals. 2) Apply filtering when metrics are added or removed. 3) Do we need to use import com.yammer.metrics.core.MetricsRegistryListener; ??

[mimaison]

yes x 3

[OwenCorrigan76]

@mimaison This is what I'm trying for the init method in YammerPrometheusMetricsReporter, using the MetricsRegistryListener import. Am I on the right track with some tweaks or am I way off? Thanks

  @Override
    public void init(VerifiableProperties props) {
        PrometheusMetricsReporterConfig config = new PrometheusMetricsReporterConfig(props.props(), registry);
        Metrics.defaultRegistry().addListener((new MetricsRegistryListener() {
            @Override
            public void onMetricAdded(MetricName metricName, Metric metric) {
                String prometheusMetricName = YammerMetricsCollector.metricName(metricName);
                System.out.println("prometheusMetricName: " + prometheusMetricName);
                if (config.isAllowed(prometheusMetricName)) {
                    registry.register(new YammerMetricsCollector(config));
                    LOG.debug("Added metric {}", prometheusMetricName);
                } else {
                    LOG.trace("Ignoring metric {} as it does not match the allowlist", prometheusMetricName);
                }
            }
            @Override
            public void onMetricRemoved(MetricName metricName) {
//                String prometheusMetricName = YammerMetricsCollector.metricName(metricName);
//              //  registry.unregister(prometheusMetricName);
//                LOG.debug("Removed metric {}", prometheusMetricName);
            }
        }));
        registry.register(new YammerMetricsCollector(config));
        LOG.debug("YammerPrometheusMetricsReporter configured with {}", config);
    }

[mimaison]

Yes MetricsRegistryListener is the class to use but I'm really confused by the code you shared. I'm not sure I follow what you are trying to do.