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

tune histogram buckets for wsgi and requests latency (milliseconds) #590

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

Conversation

verterok
Copy link
Contributor

@verterok verterok commented Jan 6, 2023

This is more a exploratory thing, I'm not entirely sure about the correctness.
I based the new values in that we don't usually care if something takes 10 or 50ms, so the first bucket is 50.
the rest of the buckets are based on the same idea and heavily based on our timeouts...which might not be suitable for others(or maybe it is?)

@bloodearnest
Copy link
Contributor

oh lord, how I dislike prometheus.

I did wonder about providing away for the WSGI app to define it's expected latency profile. But prometheus_client's design makes it horrid, as you have to declare everything as classes at import.

Sorry for the mess :(

Over here in non-C land, OpenTelemetry has grown up, and whilst not perfect, is much nicer for this kind of thing. Would be worth looking at, IMO.

Copy link
Contributor

@maxiberta maxiberta left a comment

Choose a reason for hiding this comment

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

I wonder if/how this affects existing histograms, i.e. is this backwards compatible? Can existing histogram metrics in prometheus suddenly change their bucket definition without affecting queries?

Also, do we really need to explicitly add and entry for "infinity"? The online docs show the use of <basename>_bucket{le="+Inf"}.

@verterok
Copy link
Contributor Author

verterok commented Jan 6, 2023

I wonder if/how this affects existing histograms, i.e. is this backwards compatible? Can existing histogram metrics in prometheus suddenly change their bucket definition without affecting queries?

if it breaks existing metrics, we still have the statsd based metrics.

Also, do we really need to explicitly add and entry for "infinity"? The online docs show the use of <basename>_bucket{le="+Inf"}.
No, I was just being explicit, but can remove it.

My question was more about the values makes sense or we should be more granular?

@verterok
Copy link
Contributor Author

verterok commented Jan 6, 2023

Hey!

Sorry for the mess :(

Nothing to be sorry about!

Over here in non-C land, OpenTelemetry has grown up, and whilst not perfect, is much nicer for this kind of thing. Would be worth looking at, IMO.

Oh, the endless possibilities of non-C land, how's out there in the wild?
Have OpenTelemetry in my todo/look-at list, but there is always something else to take care of (J)

Cheers!

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.

4 participants