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

Add proposal for integrating Bridge with Metrics Reporter #143

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

fvaleri
Copy link
Contributor

@fvaleri fvaleri commented Dec 19, 2024

Add proposal for integrating Bridge with Metrics Reporter.

@fvaleri fvaleri requested review from mimaison, OwenCorrigan76 and a team December 19, 2024 15:41
090-integrate-bridge-with-metrics-reporter.md Show resolved Hide resolved
090-integrate-bridge-with-metrics-reporter.md Outdated Show resolved Hide resolved
090-integrate-bridge-with-metrics-reporter.md Outdated Show resolved Hide resolved
090-integrate-bridge-with-metrics-reporter.md Outdated Show resolved Hide resolved
090-integrate-bridge-with-metrics-reporter.md Outdated Show resolved Hide resolved
090-integrate-bridge-with-metrics-reporter.md Outdated Show resolved Hide resolved
090-integrate-bridge-with-metrics-reporter.md Outdated Show resolved Hide resolved
Signed-off-by: Federico Valeri <[email protected]>
@fvaleri
Copy link
Contributor Author

fvaleri commented Dec 20, 2024

@ppatierno thanks for the review. I reworked the proposal to always present the Bridge changes first, and addressed the other comments.

@fvaleri fvaleri force-pushed the bridge-metrics branch 5 times, most recently from 7507129 to e9bb397 Compare December 20, 2024 17:01
Signed-off-by: Federico Valeri <[email protected]>
@fvaleri fvaleri requested a review from a team January 3, 2025 09:37
Signed-off-by: Federico Valeri <[email protected]>
090-integrate-bridge-with-metrics-reporter.md Outdated Show resolved Hide resolved
090-integrate-bridge-with-metrics-reporter.md Outdated Show resolved Hide resolved
090-integrate-bridge-with-metrics-reporter.md Outdated Show resolved Hide resolved
090-integrate-bridge-with-metrics-reporter.md Outdated Show resolved Hide resolved
If the property is not specified or the file is not found, the Bridge will fall back to the hard coded configuration.
This feature is not strictly required to support the Strimzi Metrics Reporter, but will be used by the Cluster Operator.

### Cluster Operator integration
Copy link
Member

Choose a reason for hiding this comment

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

This section is about the integration within the Cluster Operator which will be in charge of creating the application.properties file for the bridge but what about when the bridge runs as standalone on bare metal or VMs. In order to use the Strimzi metrics reporter, the user would need to set other parameters like disabling the JMX reporter (auto.include.jmx.reporter=false), disabling the Prometheus listener to avoid clashing with the port 8080 used by the bridge itself (prometheus.metrics.reporter.listener.enable=false).
It would be a bridge documentation update but I would mention the needed configuration here as well, maybe having a dedicated section for standalone bridge.

Copy link
Contributor Author

@fvaleri fvaleri Jan 8, 2025

Choose a reason for hiding this comment

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

There is an example in "Metrics configuration types" paragraph, but I will improve it. For auto.include.jmx.reporter there is no mention in the Strimzi Metrics Reporter proposal, so we will leave that to the user. Additionally, that property will be removed in Kafka 4.0, and the user would need to explicitly add the JMX metrics plugin when setting any other custom plugin (See #11019).

Copy link
Member

Choose a reason for hiding this comment

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

But we could end in a Strimzi version with Kafka 3.9 support, so the auto.include.jmx.reporter would be still needed, or?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given it's deprecated and not required to enable the Strimzi Metrics Reporter, I would leave this configuration to the user through .spec.kafka.config. Wdyt?

Signed-off-by: Federico Valeri <[email protected]>
Signed-off-by: Federico Valeri <[email protected]>
Copy link
Contributor

@PaulRMellor PaulRMellor left a comment

Choose a reason for hiding this comment

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

The overall approach makes sense.
I left a few suggestions, including a couple of questions where I wasn't clear on parts of the proposal.

090-integrate-bridge-with-metrics-reporter.md Outdated Show resolved Hide resolved
090-integrate-bridge-with-metrics-reporter.md Outdated Show resolved Hide resolved
090-integrate-bridge-with-metrics-reporter.md Outdated Show resolved Hide resolved
090-integrate-bridge-with-metrics-reporter.md Outdated Show resolved Hide resolved
090-integrate-bridge-with-metrics-reporter.md Outdated Show resolved Hide resolved
090-integrate-bridge-with-metrics-reporter.md Outdated Show resolved Hide resolved
090-integrate-bridge-with-metrics-reporter.md Outdated Show resolved Hide resolved
090-integrate-bridge-with-metrics-reporter.md Outdated Show resolved Hide resolved
090-integrate-bridge-with-metrics-reporter.md Outdated Show resolved Hide resolved
090-integrate-bridge-with-metrics-reporter.md Outdated Show resolved Hide resolved
Signed-off-by: Federico Valeri <[email protected]>
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.

3 participants