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

[LI-HOTFIX] Add quota bound sensor #122

Open
wants to merge 1 commit into
base: 2.4-li
Choose a base branch
from

Conversation

lmr3796
Copy link

@lmr3796 lmr3796 commented Mar 16, 2021

This patch adds QuotaBound sensor and QuotaUtilization sensor, in
addition to the existing byte-rate & throttle-count sensors.
The QuotaBound sensor records the value of of
org.apache.kafka.common.metrics.Quota#bound if it exists.

This process happens on the
kafka.server.ClientQuotaManager#recordAndGetThrottleTimeMs code path,
where quota check actually takes place.

TICKET = N/A
LI_DESCRIPTION = LIKAFKA-35289
EXIT_CRITERIA = When upstream implement similar sensors

def nullResultAsNone[X, Y](f: X => Y): X => Option[Y] = (x: X) => Option(f(x))
// Quotas may 1) not be configured or 2) not be applicable to all metrics,
// so it's necessary to check for the bound's validity beforehand
def quotaBound =
Copy link

Choose a reason for hiding this comment

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

Since the clientMetric.config may have a null quota field, but the quota's bound field should always be available, can we simply check if the clientclientMetric.config.quota field is null or not?
We should always report even if its value is 0. Also, we shouldn't worry about the isNaN case.

Copy link
Author

Choose a reason for hiding this comment

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

I think you're right on 0. Fixed it.
But I think NaN is a safety check here so I still kept it.

@gitlw
Copy link

gitlw commented Mar 17, 2021

Thanks for the PR @lmr3796
Besides the bound as bytes/s, we also want to show the utilization ratio, i.e. the usage / bound.

@lmr3796
Copy link
Author

lmr3796 commented Mar 19, 2021

@gitlw I've also added utilization rate

@lmr3796 lmr3796 force-pushed the add-bound-metrics branch from 5ee8da1 to 8039f6b Compare March 19, 2021 08:10
Copy link

@gitlw gitlw 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 this change!

core/src/main/scala/kafka/server/ClientQuotaManager.scala Outdated Show resolved Hide resolved
@lmr3796 lmr3796 force-pushed the add-bound-metrics branch 2 times, most recently from 194596b to 0d36932 Compare March 30, 2021 01:40
@lmr3796 lmr3796 force-pushed the add-bound-metrics branch from 0d36932 to ecc1526 Compare April 12, 2021 18:19
This patch adds QuotaBound sensor and QuotaUtilization sensor, in
addition to the existing byte-rate & throttle-count sensors.
The QuotaBound sensor records the value of of
`org.apache.kafka.common.metrics.Quota#bound` if it exists.

This process happens on the
`kafka.server.ClientQuotaManager#recordAndGetThrottleTimeMs` code path,
where quota check actually takes place.

TICKET = N/A
LI_DESCRIPTION = LIKAFKA-35289
EXIT_CRITERIA = When upstream implement similar sensors
@lmr3796 lmr3796 force-pushed the add-bound-metrics branch from ecc1526 to 51cf430 Compare April 22, 2021 21:18
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