cockroachdb / cockroach

CockroachDB — the cloud native, distributed SQL database designed for high availability, effortless scale, and control over data placement.
https://www.cockroachlabs.com
Other
30.17k stars 3.82k forks source link

pkg/util/metric: merge all histogram implementations into a single implementation #116584

Open abarganier opened 11 months ago

abarganier commented 11 months ago

Is your feature request related to a problem? Please describe. Today, we have 5 different histogram implementations:

With so many implementations, it's no wonder that we find ourselves dealing with histogram bugs lately. Updating the code requires one to be very thorough, updating 5 different implementations where the "right" thing to do isn't always obvious. It's easy to miss something and introduce bugs. For example, just recently I can recall a few instances where bugs were introduced to the histogram library:

Histograms are critical to customer trust. This level of toil is unacceptable, and action needs to be taken to improve our situation here.

Describe the solution you'd like Many of these histograms mostly wrap Histogram, with small differences. Does an entirely separate implementation need to exist? Instead, I propose we:

Jira issue: CRDB-34675

jbowens commented 10 months ago

My understanding is that the primary histogram type tightly couples collection of histogram data with its reporting through timeseries / grafana. I think we should consider decoupling those two responsibilities. We can still offer convenience facilities for composing the two. Decoupling will allow Pebble, the go runtime, and possibly other dependencies to collect their own histogram data without maintaining separate reporting types.