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

feat(core): histograms: support observing values a non-integral number of times #637

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dawagner
Copy link

@dawagner dawagner commented Mar 1, 2023

Observing a given value multiple times can be performed either by calling Observe multiple times or calling ObserveMultiple. The latter option is not very convenient because the caller has to keep track of existing buckets and perform bucketization.

Observing a given value a non-integral number of times can also be done using ObserveMultiple (even though that was probably not intended) because the bucket_increments argument is a vector of doubles. It can't be done by calling Observe because you can't call a function a fraction of a time.

Accumulating non-integral counts in buckets makes sense if what you are measuring is dimensionful: e.g. keeping track of how long (in seconds) an object is moving at certain (bucketed) speeds. If you observe that the object has moved at speed X for 1.5s, you'll want to increase the bucket corresponding to X by 1.5.

This commit adds an argument to Observe (defaulting to 1.0 for backward compatibility) that makes it possible increment the bucket corresponding to the value by a non-integral quantity. It can be seen as a generalization of calling Observe in a loop, for a non-integral number of times. The count is incremented by the quantity and sum is incremented by value * quantity to preserve the semantic of the count and of the sum (and then of the computed mean), which is also consistent with what would happen if Observe was called in a loop.

Verified

This commit was signed with the committer’s verified signature.
…r of times

Observing a given value multiple times can be performed either by calling
Observe multiple times or calling ObserveMultiple. The latter option is not
very convenient because the caller has to keep track of existing buckets and
perform bucketization.

Observing a given value a non-integral number of times can also be done using
ObserveMultiple (even though that was probably not intended) because the
bucket_increments argument is a vector of doubles. It can't be done by calling
Observe because you can't call a function a fraction of a time.

Accumulating non-integral counts in buckets makes sense if what you are
measuring is dimensionful: e.g. keeping track of how long (in seconds) an
object is moving at certain (bucketed) speeds. If you observe that the object
has moved at speed X for 1.5s, you'll want to increase the bucket corresponding
to X by 1.5.

This commit adds an argument to Observe (defaulting to 1.0 for backward
compatibility) that makes it possible increment the bucket corresponding to the
value by a non-integral quantity. It can be seen as a generalization of calling
Observe in a loop, for a non-integral number of times. The count is incremented
by the quantity and sum is incremented by value * quantity to preserve the
semantic of the count and of the sum (and then of the computed mean), which is
also consistent with what would happen if Observe was called in a loop.
@dawagner
Copy link
Author

dawagner commented Mar 6, 2023

I should also point out that this overload also makes it much easier to observe a given value an integral (> 1) number of times, compared to ObserveMultiple and makes it scalable compared to calling Observe in a loop.

Alternatively, all counts in all methods should be unsigned integers. Even so, it makes sense to have a quantity (even if it's an unsigned integer) argument to Observe.

@dawagner
Copy link
Author

up ? @gjasny , may you have a look, please?

@BenjaminW3
Copy link
Contributor

Technically this is fine, but none of the official prometheus client lib implementations provide this overload (I looked at Ruby, Rust and Go).
@gjasny What do you think? I do not see anything that would speek against this addition.

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