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

Weighted histogram #155

Merged
merged 3 commits into from Nov 23, 2023
Merged

Conversation

x0id
Copy link
Contributor

@x0id x0id commented Aug 20, 2023

The prometheus_histogram:observe_n/3,4,5 adds limited support for the weighted histograms. It accepts the extra argument "Weight" which is limited to integer numbers to fit existing bucket counters data type. It may be, for example, the number of time ticks when the recent value was observed, allowing for better accuracy when measurements are irregular.

In general, the weight could be any number, but this is difficult to implement since the overall Prometheus histogram model is based on the bucket counters concept, where the counters are expected to be non-negative integers. That's why the new function is limited to integer weights only, which is still useful in many applications.

@saleyn
Copy link

saleyn commented Nov 3, 2023

@deadtrickster , could you approve this one? It is a very useful patch.

@deadtrickster
Copy link
Owner

surprised by Value*Weight - Value expected to be averaged by caller or observe/n called as soon as Value changes? Weight means exact measurements count? Weights are confusing to me because of neural networks and other math stuff. Could it be Count? Why not to call existing observe on each observation?

@x0id
Copy link
Contributor Author

x0id commented Nov 17, 2023

surprised by Value*Weight - Value expected to be averaged by caller or observe/n called as soon as Value changes?

It may be averaged, or it may be instant. My case is second one. Anyway, the value is still used to calculate the bucket. Disregarding the weight.

Weight means exact measurements count? Weights are confusing to me because of neural networks and other math stuff. Could it be Count?

I don't care of the naming. It could be named count. But the weight is a generalization of count, sorry for math again :)

Why not to call existing observe on each observation?

Because to keep the intention, I would have to call observe N times for a single observation. Remember, we deal with histogram, not the gauges. The values is mapped into the bucket number (or the bucket interval, strictly saying). And I need it to be counted in the bucket more than one time per observation.

@ikavgo
Copy link
Collaborator

ikavgo commented Nov 17, 2023

In prometheus histograms have counts, not weights.

ok, now it's more clear that you have just single event that comes with value and count.
I'm ok merging it iff weights renamed to counts and @doc strings are very clear what value ends up in the bucket - observed or observed * count and why/

@x0id
Copy link
Contributor Author

x0id commented Nov 23, 2023

I'm ok merging it if weights renamed to counts and @doc strings are very clear what value ends up in the bucket - observed or observed * count and why/

Renamed weights to counts, fixed the doc.
Please let me know if anything else should be corrected.

@ikavgo
Copy link
Collaborator

ikavgo commented Nov 23, 2023

Great! I think commits must be signed

@x0id
Copy link
Contributor Author

x0id commented Nov 23, 2023

Great! I think commits must be signed

Is this your requirement? I've never signed my commits before. I don't even have a GPG/PGP or other signature. Why is this important?

@deadtrickster
Copy link
Owner

It's a requirement for this repo, yes. I think it was made so on request of somebody wanting to use in their org. Years ago

x0id and others added 3 commits November 23, 2023 14:50
This expected to help in collecting weighted histograms,
when we need to update bucket adding given positive integer number
(aka wight or count)
- add support to non-integer values
- count the sum correctly (with weights)
- add test cases
- adjust documentation
@x0id
Copy link
Contributor Author

x0id commented Nov 23, 2023

Commits signed.

@ikavgo ikavgo self-requested a review November 23, 2023 21:17
@deadtrickster deadtrickster merged commit d1aad4a into deadtrickster:master Nov 23, 2023
14 checks passed
@deadtrickster
Copy link
Owner

❤️

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

4 participants