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

Is WindowedCount the right stat type to use in MetricsResourceMethodApplicationListener? #344

Open
ericwush opened this issue Aug 2, 2022 · 6 comments

Comments

@ericwush
Copy link
Member

ericwush commented Aug 2, 2022

By default, an application is configured to keep two samples in a SampledStat, e.g., WindowedCount, each sample maintains a 30-second window. Typically, metrics are exposed by JmxReporter. When the value of an MBean is being retrieved, it calls the stat's measure method. In WindowedCount, measure simply combines all data points in all (two) samples to get the sum. Unless the MBean retrievals are well aligned with the start of a window with the intervals being (window size * # of samples), I don't see how it could report the right count.

For instance:
T: 0----5----[10]----15----[20]----25----[30]
W: |-----1-----|------2------|------3-------|

Let's assume the window size is 10, the metrics get collected every 10 seconds.
Collection 1 at 15: it returns the sum of (full window 1 and half of window 2), i.e., 0-15
Collection 2 at 25: it returns the sum of (full window 2 and half of window 3), i.e., 10-25
In this example, the values between 10-15 are repeatedly reported.

Can anyone please verify if this is the right understanding?

@ehumber
Copy link
Member

ehumber commented Aug 3, 2022

@splett2 Any thoughts on the question from Eric?

@splett2
Copy link
Member

splett2 commented Aug 3, 2022

seems reasonable to me. WindowedCount in the kafka code base is usually used as a rate stat in combination with a cumulative count (the count) to construct a meter.

I would probably rename the existing CumulativeCount to request-rate and then the new cumulative count metric to request-total.

@ericwush
Copy link
Member Author

ericwush commented Aug 3, 2022

Thanks @splett2. I will rename it to request-total and leave the existing ones to code owner to change because we need to make sure anyone using those metrics for alerts should be notified.

@splett2
Copy link
Member

splett2 commented Aug 3, 2022

@ehumber
we should probably just deprecate and eventually remove the existing request-count metric. AFAIK it should express an equivalent value to the request-rate metric, since both are using a WindowedCount as the underlying Stat.

@ericwush
Copy link
Member Author

ericwush commented Aug 3, 2022

Another approach is rely on request-total and calculate the rate in dd? That is something I am going to try.

Also, response-byte-rate and response-size-avg are duplicate too?

@dimitarndimitrov
Copy link
Member

CC @rigelbm

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

No branches or pull requests

4 participants