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 CountSinceLastMeasurement and TotalSinceLastMeasurement that resets everytime its measured #32

Merged
merged 6 commits into from
Sep 13, 2024

Conversation

m-nagarajan
Copy link
Collaborator

@m-nagarajan m-nagarajan commented Sep 12, 2024

  1. add CountSinceLastMeasurement that resets the count everytime its measured
    Simple version of SampledCount, this aggregates counts as a single sample and gets reset to 0 every time it is measured. The sample window is just the time between measurements unlike SampledCount which can be defined by number of events or elapsed time. Measure every second to get count per second. Measure every minute to get count per minute. Measure adhoc to get the delta count since the last measurement.
  2. add TotalSinceLastMeasurement that resets the total everytime its measured

These classes doesn't support quota currently as it resets everytime it's measured which probably need a refactoring where the measure call triggered from the quota path does not cause a reset, and only the measure call from the MetricsReporter end up triggering the reset.

@m-nagarajan m-nagarajan changed the title add SimpleSampledCount that resets the count everytime its measured add SimpleSampledCount and SimpleSampledTotal that resets everytime its measured Sep 12, 2024
@FelixGV
Copy link
Contributor

FelixGV commented Sep 13, 2024

Thanks, I think this is a useful change.

What do you think about calling these two classes CountSinceLastMeasurement and TotalSinceLastMeasurement? It doesn't roll off the tongue as much, but I feel it's more descriptive?

Also, I am thinking about two potential scope creeps, which probably do not need to be addressed in this PR, but I'd like to hear if you have thoughts on them...

  1. I would like to be able to build a Rate and OccurrenceRate based on these ...SinceLastMeasurement stats. Why? Because I think their simplicity should make them even more efficient than SampledCount and SampledTotal. But...

  2. Because rates are often used as part of quotas, we should also think about how these ...SinceLastMeasurement stats interact with quotas. In this PR's proposed implementation, I think quotas would be broken, which is maybe ok because it seems uncommon to have a quota configured on something else than a rate, but anyhow... To make interactions with quotas work, we would probably need a refactoring where the measure call triggered from the quota path does not cause a reset, and only the measure call from the MetricsReporter end up triggering the reset. I don't think we need to fix this in this PR, but maybe we should make the incompatibility explicit by having the two new stats implement Initializable and have them throw an exception if the passed in MetricConfig has quotas enabled?

@m-nagarajan
Copy link
Collaborator Author

Thanks @FelixGV for the comments

What do you think about calling these two classes CountSinceLastMeasurement and TotalSinceLastMeasurement? It doesn't roll off the tongue as much, but I feel it's more descriptive?

Though the names suggested feels a bit too descriptive, I believe it's better than being cryptic which was the case before. I am onboard. thanks.

  1. Because rates are often used as part of quotas, we should also think about how these ...SinceLastMeasurement stats interact with quotas. In this PR's proposed implementation, I think quotas would be broken, which is maybe ok because it seems uncommon to have a quota configured on something else than a rate, but anyhow... To make interactions with quotas work, we would probably need a refactoring where the measure call triggered from the quota path does not cause a reset, and only the measure call from the MetricsReporter end up triggering the reset. I don't think we need to fix this in this PR, but maybe we should make the incompatibility explicit by having the two new stats implement Initializable and have them throw an exception if the passed in MetricConfig has quotas enabled?

Took a look at the code and it needs some refactoring like you mentioned as it's the same set functions that gets called on both the paths. For now, made these 2 new classes to throw an exception during init() if config has quota in it to make it explicit.

  1. I would like to be able to build a Rate and OccurrenceRate based on these ...SinceLastMeasurement stats. Why? Because I think their simplicity should make them even more efficient than SampledCount and SampledTotal. But...

That is a good suggestion. I will take a stab at this along with the above refactoring with a separate PR.

Copy link
Contributor

@FelixGV FelixGV left a comment

Choose a reason for hiding this comment

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

Thanks Manoj, looks great!

@m-nagarajan m-nagarajan changed the title add SimpleSampledCount and SimpleSampledTotal that resets everytime its measured add CountSinceLastMeasurement and TotalSinceLastMeasurement that resets everytime its measured Sep 13, 2024
@FelixGV FelixGV merged commit 0dc7fb1 into tehuti-io:master Sep 13, 2024
1 check passed
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.

2 participants