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

Metrics #39

Merged
merged 23 commits into from
Jul 26, 2023
Merged

Metrics #39

merged 23 commits into from
Jul 26, 2023

Conversation

SamBarker
Copy link

@SamBarker SamBarker commented Jun 27, 2023

This PR adds the metrics outlined in proposal 47.

Note there are a couple of name changes in the implementation compared to the proposal:

Proposal Implementation
io.strimzi.kafka.quotas:type=LocalThrottleFactor,name=ThrottleFactor io.strimzi.kafka.quotas:type=ThrottleFactor,name=ThrottleFactor
io.strimzi.kafka.quotas:type=LocalThrottleFactor,name=FallbackThrottleFactorApplied io.strimzi.kafka.quotas:type=ThrottleFactor,name=FallbackThrottleFactorApplied
io.strimzi.kafka.quotas:type=LocalThrottleFactor,name=LimitViolated io.strimzi.kafka.quotas:type=ThrottleFactor,name=LimitViolated
io.strimzi.kafka.quotas:type=ClusterVolumeSouce,name=ActiveBrokers io.strimzi.kafka.quotas:type=VolumeSouce,name=ActiveBrokers
io.strimzi.kafka.quotas:type=ClusterVolumeSouce,name=ActiveLogDirs io.strimzi.kafka.quotas:type=VolumeSouce,name=ActiveLogDirs

In short LocalThrottleFactor -> ThrottleFactor and ClusterVolumeSource -> VolumeSource as the local and cluster distinctions no longer make sense (I don't think they actually did when merged the proposal but no one picked up on the fact we had lost the distinction).

The Metrics

Name Metric Type Meaning Type Tags
ThrottleFactor Gauge The current factor applied by the plug-in [0..1] ThrottleFactor observingBrokerId
FallbackThrottleFactorApplied Counter The number of times the plug-in has transitioned to using the fall back factor ThrottleFactor observingBrokerId
LimitViolated Counter A count of the number logDirs which violate the configured limit ThrottleFactor observingBrokerId
ActiveBrokers Gauge The current number of brokers returned by the describeCluster rpc VolumeSource observingBrokerId
ActiveLogDirs Gauge The number of logDirs returned by the describeLogDirs RPC VolumeSource observingBrokerId
available_bytes Gauge The number of available bytes returned by the describeLogDirs RPC VolumeSource [observingBrokerId, remoteBrokerId, logDir]
consumed_bytes Gauge The number of consumed bytes returned by the describeLogDirs RPC VolumeSource [observingBrokerId, remoteBrokerId, logDir]

Tag definitions

Tag Definition
observingBrokerId The BrokerId of the broker node executing the plug-in
remoteBrokerId The BrokerId of the broker node hosting the logDir
logDir The path to the specific logDir as returned by the describeLogDirs RPC

…served.

Signed-off-by: Sam Barker <sbarker@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
…metric.

Signed-off-by: Sam Barker <sbarker@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
…FactorApplied

Signed-off-by: Sam Barker <sbarker@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
@robobario
Copy link

robobario commented Jun 27, 2023

Instead of having the static metricName called from a few spots could we instead pass in some class with the metrics bits on it? Like new QuotaPluginMetrics(defaultTags) and call methods on it like metrics.volumeSource().incrementConsumedBytes(xyz). Then we could avoid some of the defaultTags fields and hopefully contain the metricName logic.

edit: looking at it, that's bit of a hefty refactor now with not much gain, happy with it as is and we could revisit later

… time.

Signed-off-by: Sam Barker <sbarker@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
@scholzj scholzj added this to the 0.3.0 milestone Jun 28, 2023
Copy link
Member

@tombentley tombentley left a comment

Choose a reason for hiding this comment

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

Thanks @SamBarker, there are a few nits, but this pretty good over all.

available_bytes -> AvailableBytes
consumed_bytes -> ConsumedBytes

Signed-off-by: Sam Barker <sbarker@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
The metric names are Yammer metric names rather than mBean names per se.

Signed-off-by: Sam Barker <sbarker@redhat.com>
Complies with https://docs.oracle.com/javase/8/docs/api/javax/management/ObjectName.html and excludes `*` and `?` as they indicate patterns and shouldn't appear in object names.

Signed-off-by: Sam Barker <sbarker@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
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.

LGTM.

Copy link
Member

@tombentley tombentley left a comment

Choose a reason for hiding this comment

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

A few more nits on the code. The other thing we should do is update the README to mention the availability and meaning of the metrics (you can probably copy and paste some of what you wrote from the PR)

Signed-off-by: Sam Barker <sbarker@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
Copy link
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

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

LGTM! Nice tests!

Signed-off-by: Sam Barker <sbarker@redhat.com>
Copy link
Member

@tombentley tombentley left a comment

Choose a reason for hiding this comment

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

Overall this LGTM, thanks.

Signed-off-by: Sam Barker <sbarker@redhat.com>
Can't find any reference to that actually being a supported configuration mechanism.

Signed-off-by: Sam Barker <sbarker@redhat.com>
Signed-off-by: Sam Barker <sbarker@redhat.com>
Also AbstractConfig applies a namespace so need to use `originals` to get at the `broker.id`.

Signed-off-by: Sam Barker <sbarker@redhat.com>
@SamBarker
Copy link
Author

@ppatierno @tombentley I don't believe there is anything outstanding on this PR, if I'm right could one of you kindly hit the merge button? Otherwise please let me know what else needs looking at?

@ppatierno ppatierno merged commit d8327a2 into strimzi:main Jul 26, 2023
5 checks passed
@SamBarker SamBarker deleted the metrics branch July 26, 2023 09:45
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.

None yet

6 participants