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.

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

Separate logic for Kafka and Yammer metrics #49

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

mimaison
Copy link
Contributor

@mimaison mimaison commented Oct 4, 2024

This fixes #48

This PR separates the Kafka and Yammer specific metric logic. This ensures that when usingKafkaPrometheusMetricsReporter, only classes present in kafka-clients are used. On the other hand YammerPrometheusMetricsReporter can use all dependencies present in the Kafka distribution as it's only used on the server side.

@mimaison mimaison added this to the 0.1.0 milestone Oct 4, 2024
Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

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

I haven't looked much at the code itself in the PR but tested with the Strimzi HTTP bridge where we found the issue. Everything seems to work fine now but the metrics reporter configuration should be documented in the bridge repo. I opened strimzi/strimzi-kafka-bridge#932 for that.

@mimaison mimaison requested a review from k-wall October 7, 2024 08:50
@k-wall
Copy link

k-wall commented Oct 7, 2024

@mimaison could you add a sentence or two to the PR description outlining at a high level how the problem is solved. It should help reviewers understand the intent.

Copy link

@k-wall k-wall left a comment

Choose a reason for hiding this comment

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

The change made me wonder if there should be a separate Maven artefact one for the client use-case and one for the broker use case. That'd keep the server side concerns completely out of the client's classpath.

I think what you have is good enough for the initial versions.

@mimaison mimaison merged commit c3d5721 into strimzi:main Oct 8, 2024
7 checks passed
@mimaison mimaison deleted the issue-48 branch October 8, 2024 18:43
scholzj pushed a commit that referenced this pull request Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using KafkaPrometheusMetricsReporter should only require kafka-clients
3 participants