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.

Sign up for GitHub

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

Jump to bottom

Add a Prometheus-compatible Histogram function, and convert the vmrange label to the le label #45

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add a Prometheus-compatible Histogram function, and convert the vmrange label to the le label #45

wants to merge 5 commits into from

Conversation

greyireland
Copy link

No description provided.

This was referenced Feb 7, 2023
Open
Open
Open
cristaloleg
cristaloleg reviewed Feb 19, 2023
View reviewed changes
set.go Outdated Show resolved Hide resolved
@elee1766 elee1766 mentioned this pull request May 11, 2023
Merged
Giulio2002 pushed a commit to ledgerwatch/erigon that referenced this pull request May 11, 2023
@elee1766
[Caplin] beginnings of instrumentation (#7486)
fd6acd4 
this pr is ready for review, but it is waiting on this PR 

VictoriaMetrics/metrics#45

so that we do not need to use a replace directive.
@clementnuss
Copy link

any news here ? what can be done to help get this PR merged ?

@hagen1778
Copy link
Contributor

hagen1778 commented May 23, 2023 •
edited

Hello @greyireland @clementnuss!
I don't think it is a good idea to convert VictoriaMetrics histograms in Prometheus histograms because the former histograms are sparse with dynamic buckets. While standard Prometheus histograms are static and the whole logic for processing Prometheus histograms relies on that logic. For example, if instance A will produce 10 buckets for query latency histogram, and instance B will produce 11 buckets - the aggregation of 99th latency percentile for instance A and B will be calculated incorrectly.
So in fact, this change will result into confusion and incorrect processing results. Prometheus standard histograms should have static buckets across all application instances.

@Neal
Copy link

Neal commented Jun 13, 2023

@hagen1778 that's a good point and in that case it may make more sense to add Prometheus style static buckets based Histogram in order to be fully compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cristaloleg cristaloleg cristaloleg left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@greyireland @clementnuss @hagen1778 @Neal @cristaloleg