-
Notifications
You must be signed in to change notification settings - Fork 130
Expose JVM and Kafka clients metrics #435
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
Expose JVM and Kafka clients metrics #435
Conversation
738ae11
to
85c402d
Compare
Codecov Report
@@ Coverage Diff @@
## master #435 +/- ##
============================================
+ Coverage 76.19% 76.79% +0.59%
- Complexity 260 269 +9
============================================
Files 58 59 +1
Lines 1907 1969 +62
Branches 82 82
============================================
+ Hits 1453 1512 +59
- Misses 339 342 +3
Partials 115 115
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
/retest |
1 similar comment
/retest |
85c402d
to
73f003f
Compare
Flaky |
/hold |
connection reset, maven |
Hi, the java Kafka client does not provide any metrics eg. via jmx? Do we need any docs here? |
Hi @skonto, we're using micrometer-registry-prometheus to export metrics rather than JMX, are you suggesting to use JMX?
Yes, I'm thinking to link the page: https://kafka.apache.org/documentation/#selector_monitoring, for knowing which metrics we expose, and clarifying that "-" are replaced by "_" since metric names must match We already expose metrics configurations for both consumer and producer: https://github.com/knative-sandbox/eventing-kafka-broker/blob/50e92f981fd837b142425965a828f44c56354e93/data-plane/config/100-config-kafka-broker-data-plane.yaml#L45-L47 I'll mark this PR as WIP, since I'd like to keep metrics names consistent with other projects in the Kafka area. |
73f003f
to
6ef241b
Compare
03837e5
to
8e91dec
Compare
Worked, retesting since it seems to be unstable: |
Known flake, |
@pierDipi my thinking was to review existing metrics if available (I know you are using something vert.x specific here). |
I did a POC using the JMX exporter, using an initContiner to inject the agent: pierDipi@15d1494, and discussing this with @slinkydeveloper, we concluded that having more control over what metrics we expose it's better long term, and we also need to expose custom metrics that are part of the Broker spec, so we need to use the Vert.x API anyway. For example, I'm disabling VertxPoolMetrics due to an impressive allocation rate: Also, he suggested that an agent might be heavyweight from a resource perspective. WDYT?
Thanks, I really appreciate it. |
/test pull-knative-sandbox-eventing-kafka-broker-integration-tests |
This PR is ready for reviews. |
@pierDipi: GitHub didn't allow me to request PR reviews from the following users: skonto. Note that only knative-sandbox members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
4aaed10
to
8042ffa
Compare
Rebased. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I just left a minor comment
data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/utils/BaseEnv.java
Show resolved
Hide resolved
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pierDipi, slinkydeveloper The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/unhold |
Flaky :( /retest |
/retest |
1 similar comment
/retest |
* Expose Kafka clients metrics Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Reduce frequency in sacura job Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Keep running sacura if tests fail Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Too many logs in debug mode Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Increase timeout Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Close KafkaClientMetrics binder, WebClient, Producer and Consumer Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Revert sacura changes Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Remove tracing Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Producer metrics on receiver Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Add more tests and docs Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
…o slow (knative-extensions#422) (knative-extensions#435) Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
To better understand our data plane components state, we can
export Kafka clients metrics and JVM metrics (JVM metrics are
disabled by default).
Proposed Changes
Release Note
/kind enhancement
Example metrics