Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move label filtering into KafkaPrometheusReporter and out of the KafkaPrometheusCollector #38

Merged
merged 5 commits into from
Aug 16, 2024
Merged

Move label filtering into KafkaPrometheusReporter and out of the KafkaPrometheusCollector #38

merged 5 commits into from
Aug 16, 2024

Conversation

OwenCorrigan76
Copy link
Contributor

This PR is in response to the following issue: #9
Moving the label filtering into the metricChange method means that filtering is done when the Reporter is initialised. It was previously done in the collect method.

@OwenCorrigan76 OwenCorrigan76 added this to the 0.1.0 milestone Aug 13, 2024
@OwenCorrigan76 OwenCorrigan76 force-pushed the KafkaPrometheusMetricsReporter_performance_improvements branch from f3ab94a to bf86d91 Compare August 13, 2024 10:36
mimaison
mimaison reviewed Aug 13, 2024
View reviewed changes
Copy link
Contributor

@mimaison mimaison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, I left a few comments.

src/main/java/io/strimzi/kafka/metrics/KafkaMetricsCollector.java Outdated Show resolved Hide resolved
src/main/java/io/strimzi/kafka/metrics/KafkaPrometheusMetricsReporter.java Outdated Show resolved Hide resolved
src/main/java/io/strimzi/kafka/metrics/KafkaPrometheusMetricsReporter.java Outdated Show resolved Hide resolved
src/main/java/io/strimzi/kafka/metrics/KafkaPrometheusMetricsReporter.java Show resolved Hide resolved
src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java Show resolved Hide resolved
src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java Outdated Show resolved Hide resolved
src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java Outdated Show resolved Hide resolved
src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java Outdated Show resolved Hide resolved
src/test/java/io/strimzi/kafka/metrics/KafkaPrometheusMetricsReporterTest.java Show resolved Hide resolved
src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java Show resolved Hide resolved
mimaison
mimaison reviewed Aug 14, 2024
View reviewed changes
Copy link
Contributor

@mimaison mimaison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates! I looks good overall. I left a few more suggestions but we're almost there.

src/main/java/io/strimzi/kafka/metrics/KafkaPrometheusMetricsReporter.java Outdated Show resolved Hide resolved
src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java Outdated Show resolved Hide resolved
src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java Outdated Show resolved Hide resolved
src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java Outdated Show resolved Hide resolved
src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java Show resolved Hide resolved
src/test/java/io/strimzi/kafka/metrics/KafkaPrometheusMetricsReporterTest.java Outdated Show resolved Hide resolved
src/test/java/io/strimzi/kafka/metrics/KafkaPrometheusMetricsReporterTest.java Outdated Show resolved Hide resolved
src/test/java/io/strimzi/kafka/metrics/KafkaPrometheusMetricsReporterTest.java Outdated Show resolved Hide resolved
src/test/java/io/strimzi/kafka/metrics/MetricWrapperTest.java Show resolved Hide resolved
mimaison
mimaison reviewed Aug 16, 2024
View reviewed changes
Copy link
Contributor

@mimaison mimaison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made another pass and left a few more minor suggestions

src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java Outdated
Measurable measurable = (config, now) -> value;
return new KafkaMetric(
KafkaMetric metric = new KafkaMetric(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the same naming as in newNonNumericMetric()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Changed to kafkaMetric

src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java Outdated
new Object(),
new MetricName(name, group, "", tagsMap),
new MetricName(metricName.name(), metricName.group(), "", tagsMap),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use metricName here like in newNonNumericMetric()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I missed that. Now using metricName

src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java Outdated
collector.addMetric(buildMetric("name", "other", 2.0));
metrics = collector.collect();
assertEquals(0, metrics.size());
// Add a metric
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Should we keep gerunds so all comments are similar?
Same in the other comment you updated below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to Adding etc.

src/test/java/io/strimzi/kafka/metrics/KafkaPrometheusMetricsReporterTest.java Outdated
reporter.close();
}

@Test
public void testMultipleReporters() throws Exception {
Map<String, String> configs = new HashMap<>();
configs.put(PrometheusMetricsReporterConfig.LISTENER_CONFIG, "http://:0");
configs.put(PrometheusMetricsReporterConfig.ALLOWLIST_CONFIG, "kafka_server_group_name.*");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to set this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed here. Removed

src/main/java/io/strimzi/kafka/metrics/KafkaPrometheusMetricsReporter.java Outdated
public void metricChange(KafkaMetric metric) {
kafkaMetricsCollector.addMetric(metric);
String prometheusName = MetricWrapper.prometheusName(this.prefix, metric.metricName());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.prefix -> prefix

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@OwenCorrigan76
Copy link
Contributor Author

@mimaison Thanks for the extra pass and comments. Hopefully there's nothing missed now.

mimaison
mimaison reviewed Aug 16, 2024
View reviewed changes
src/main/java/io/strimzi/kafka/metrics/KafkaPrometheusMetricsReporter.java Outdated
private final PrometheusRegistry registry;
@SuppressFBWarnings({"UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR"}) // This field is initialized in the configure method
private KafkaMetricsCollector kafkaMetricsCollector;
private KafkaMetricsCollector collector;
@SuppressFBWarnings({"UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR"}) // This field is initialized in the init method
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

init -> configure

mimaison
mimaison reviewed Aug 16, 2024
View reviewed changes
src/main/java/io/strimzi/kafka/metrics/KafkaPrometheusMetricsReporter.java Outdated
@SuppressFBWarnings({"UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR"}) // This field is initialized in the configure method
private Optional<HTTPServer> httpServer;
@SuppressFBWarnings({"UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR"}) // This field is initialized in the setPrefix method
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setPrefix -> contextChange

@OwenCorrigan76
fix mistakes
8bc61a8 
Signed-off-by: Owen <[email protected]>
scholzj
scholzj approved these changes Aug 16, 2024
View reviewed changes
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.

mimaison
mimaison approved these changes Aug 16, 2024
View reviewed changes
Copy link
Contributor

@mimaison mimaison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mimaison mimaison merged commit dfcde19 into strimzi:main Aug 16, 2024
7 checks passed
@mimaison mimaison mentioned this pull request Aug 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scholzj scholzj scholzj approved these changes

@mimaison mimaison mimaison approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.1.0
Development

Successfully merging this pull request may close these issues.
3 participants
@OwenCorrigan76 @mimaison @scholzj