Start drafting weighted and timing histograms #109277

MikeSpreitzer · 2022-04-04T07:04:46Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR introduces two new kinds of histograms, weighted histograms and timing histograms. A timing histogram has the interface of a gauge, but keeps track of the time that the variable spent in each of the ranges defined by the bucket boundaries. A timing histogram is built on a weighted histogram, which is like a regular histogram but its Observe method takes a weight as well as a value. Due to the limitations of the current model (in OpenMetrics) for histograms, the weight has to be an unsigned integer. The timing histograms are intended to replace the existing sample-and-watermark histograms in k/apiserver/pkg/util/flowcontrol with something less complex to consume and less costly at runtime to update.

These new histograms are hoped to eventually migrate into prometheus. Until then, they resides in k/component-base/metrics/prometheusextension .

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

This is like #109094 but:

is factored to cover the use case of Feature Request: Permit histogram's Observe() method to take an observation count prometheus/client_golang#796 (comment) as well, and
the timing histograms implement the gauge interface, as noted in Feature Request: Permit histogram's Observe() method to take an observation count prometheus/client_golang#796 (comment) .

This is part of addressing #108272 .

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@kubernetes/sig-api-machinery-misc
/cc @wojtek-t
@beorn7
@dgrisonnet
@logicalhan

wojtek-t · 2022-04-04T12:42:53Z

staging/src/k8s.io/component-base/metrics/prometheusextension/weighted_histogram.go

+)
+
+// WeightedObserver
+type WeightedObserver interface {


@MikeSpreitzer - I think I'm not following the usecase of having weight here.
Can you point me to the usecase for it? Why is timingHistogram not enough for us?

@caibirdme gave a use case at prometheus/client_golang#796 (comment)

serathius · 2022-04-04T15:31:43Z

cc @logicalhan

MikeSpreitzer · 2022-04-04T16:55:14Z

/retest

cici37 · 2022-04-05T20:12:51Z

/remove-sig api-machinery

logicalhan · 2022-04-07T15:06:12Z

staging/src/k8s.io/component-base/metrics/prometheusextension/timing_histogram.go

+type GaugeOps interface {
+	// Set sets the Gauge to the given value.
+	Set(float64)
+	// Add(1)


nit: comments should be prefixed by the method they are commenting by convention.

yeah - let's maybe just copy comments from
https://pkg.go.dev/github.com/prometheus/client_golang/prometheus#Gauge

Some of the comments here are just placeholders, yes they need to follow conventions and be useful.

To make the relationship utterly clear, in my latest push I made the comments on the methods of GaugeOps state their equivalence to the corresponding method of Gauge.

logicalhan · 2022-04-07T15:06:17Z

staging/src/k8s.io/component-base/metrics/prometheusextension/timing_histogram.go

+	Set(float64)
+	// Add(1)
+	Inc()
+	// Sub(1)


logicalhan · 2022-04-07T15:06:17Z

staging/src/k8s.io/component-base/metrics/prometheusextension/timing_histogram.go

+	Set(float64)
+	// Add(1)
+	Inc()
+	// Sub(1)


logicalhan · 2022-04-07T15:10:43Z

staging/src/k8s.io/component-base/metrics/prometheusextension/timing_histogram.go

+}
+
+func (th *timingHistogram) SetToCurrentTime() {
+	th.update(func(oldValue float64) float64 { return th.clock.Since(time.Unix(0, 0)).Seconds() })


I'm really confused by this method, are you intending to set the gauge value to the unix timestamp or are you intending to set the th.lastSetTime to the current timestamp?

I think this method will just set the current time as the new value it will not override the lastSetTime. At least that would be the behavior of the normal SetToCurrentTime method of a gauge. I find it also a bit confusing, but I guess it was a shortcut made in client_golang to simplify the use of gauge with timestamp metrics.

This is strange to me too, but effectively this method is coming from the original prometheus interface:
https://pkg.go.dev/github.com/prometheus/client_golang/prometheus#Gauge

// SetToCurrentTime sets the Gauge to the current Unix time in seconds. SetToCurrentTime()

So it works as designed :)

I created GaugeOps from the existing Gauge interface, just removing the inclusion of Metric and Collector. Frankly, prometheus should have this interface (analogous to the way it has Observer = Histogram - {Metric, Collector}).

logicalhan · 2022-04-07T15:11:18Z

staging/src/k8s.io/component-base/metrics/prometheusextension/timing_histogram_test.go

+	"github.com/prometheus/client_golang/prometheus"
+
+	dto "github.com/prometheus/client_model/go"


these should be grouped

logicalhan · 2022-04-07T15:12:49Z

staging/src/k8s.io/component-base/metrics/prometheusextension/timing_histogram_test.go

+		v0 := value0
+		t1 := t0.Add(time.Nanosecond)
+		var v1 float64 = 0.75
+		clk.SetTime(t1)
+		th.Set(v1)
+		t2 := t1.Add(time.Microsecond)
+		var d2 float64 = 0.5
+		v2 := v1 + d2
+		clk.SetTime(t2)
+		th.Add(d2)
+		t3 := t2
+		for i := 0; i < 1000000; i++ {
+			t3 = t3.Add(time.Nanosecond)
+			clk.SetTime(t3)
+			th.Set(v2)
+		}
+		var d3 float64 = -0.6
+		v3 := v2 + d3
+		th.Add(d3)
+		t4 := t3.Add(time.Second)
+		clk.SetTime(t4)
+
+		metch := make(chan prometheus.Metric)


this test setup is really hard to read and it's not obvious to me what you are doing...

In my latest push I added a func comment hopefully explaining it.

logicalhan · 2022-04-07T15:14:24Z

staging/src/k8s.io/component-base/metrics/prometheusextension/timing_histogram_test.go

+		if want, got := uint64(t4.Sub(t0)), wroteHist.GetSampleCount(); want != got {
+			t.Errorf("Wanted %v but got %v", want, got)
+		}
+		if want, got := float64(t1.Sub(t0))*v0+float64(t2.Sub(t1))*v1+float64(t3.Sub(t2))*v2+float64(t4.Sub(t3))*v3, wroteHist.GetSampleSum(); want != got {


maybe worth adding a private method which does the Sub and casts to a float for readability just for test readability.

logicalhan · 2022-04-07T15:15:11Z

staging/src/k8s.io/component-base/metrics/prometheusextension/weighted_histogram.go

+)
+
+// WeightedObserver
+type WeightedObserver interface {


Can we have tests for weighted histogram too?

logicalhan · 2022-04-07T16:38:36Z

/triage accepted
/assign @logicalhan @dgrisonnet

dgrisonnet · 2022-04-26T11:04:17Z

New changes look good to me. I am fine with moving forward with this PR once wojtek's comments are addressed.

MikeSpreitzer · 2022-04-27T18:52:09Z

@dgrisonnet, I have three concerns with making the histogram code add text about the type into the help line:

A scrape gets two comments per metric, one about the type and one about the instance. The proposal here is to put words about the type into the line about the instance. That's a mental mismatch.
When the reader sees the words "EXPERIMENTAL METRIC TYPE", she will look for the type that is experimental. She will not see it. The # TYPE line remains ordinary.
Adding fixed words onto the description provided by the client raises the question of whether the combination will parse and make sense.

MikeSpreitzer · 2022-04-27T20:39:13Z

/retest

dgrisonnet · 2022-04-28T13:12:33Z

I understand your concerns, but sadly we can't tweak the # TYPE comment since Prometheus is using it during ingestion to identify what is the type of the metrics that it is ingesting: https://github.com/prometheus/prometheus/blob/main/model/textparse/promparse.go#L291-L305. If we were to put something other than histogram which is the type we want the new metrics to be identified as, the parser would throw an error and the scrape would fail.

To avoid any confusion for the users, maybe just pretending the actual HELP text by EXPERIMENTAL: is enough. I don't think the consumers will really care about the underlying implementation concerns that we might have. Just knowing that this metric is experimental should make them aware that it should be used with care and it might change anytime.

MikeSpreitzer · 2022-04-28T21:27:45Z

The force-push to 5f8b53ef38e makes the suggested addition to the # HELP line of timing histograms, and squashes to a single commit.

The following investigation occurred during development. Add TimingHistogram impl that shares lock with WeightedHistogram Benchmarking and profiling shows that two layers of locking is noticeably more expensive than one. After adding this new alternative, I now get the following benchmark results. ``` (base) mspreitz@mjs12 kubernetes % go test -benchmem -run=^$ -bench ^BenchmarkTimingHistogram$ k8s.io/component-base/metrics/prometheusextension goos: darwin goarch: amd64 pkg: k8s.io/component-base/metrics/prometheusextension cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz BenchmarkTimingHistogram-16 22232037 52.79 ns/op 0 B/op 0 allocs/op PASS ok k8s.io/component-base/metrics/prometheusextension 1.404s (base) mspreitz@mjs12 kubernetes % go test -benchmem -run=^$ -bench ^BenchmarkTimingHistogram$ k8s.io/component-base/metrics/prometheusextension goos: darwin goarch: amd64 pkg: k8s.io/component-base/metrics/prometheusextension cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz BenchmarkTimingHistogram-16 22190997 54.50 ns/op 0 B/op 0 allocs/op PASS ok k8s.io/component-base/metrics/prometheusextension 1.435s ``` and ``` (base) mspreitz@mjs12 kubernetes % go test -benchmem -run=^$ -bench ^BenchmarkTimingHistogramDirect$ k8s.io/component-base/metrics/prometheusextension goos: darwin goarch: amd64 pkg: k8s.io/component-base/metrics/prometheusextension cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz BenchmarkTimingHistogramDirect-16 28863244 40.99 ns/op 0 B/op 0 allocs/op PASS ok k8s.io/component-base/metrics/prometheusextension 1.890s (base) mspreitz@mjs12 kubernetes % (base) mspreitz@mjs12 kubernetes % (base) mspreitz@mjs12 kubernetes % go test -benchmem -run=^$ -bench ^BenchmarkTimingHistogramDirect$ k8s.io/component-base/metrics/prometheusextension goos: darwin goarch: amd64 pkg: k8s.io/component-base/metrics/prometheusextension cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz BenchmarkTimingHistogramDirect-16 27994173 40.37 ns/op 0 B/op 0 allocs/op PASS ok k8s.io/component-base/metrics/prometheusextension 1.384s ``` So the new implementation is roughly 20% faster than the original. Add overlooked exception, rename timingHistogram to timingHistogramLayered Use the direct (one mutex) style of TimingHistogram impl This is about a 20% gain in CPU speed on my development machine, in benchmarks without lock contention. Following are two consecutive trials. (base) mspreitz@mjs12 prometheusextension % go test -benchmem -run=^$ -bench Histogram . goos: darwin goarch: amd64 pkg: k8s.io/component-base/metrics/prometheusextension cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz BenchmarkTimingHistogramLayered-16 21650905 51.91 ns/op 0 B/op 0 allocs/op BenchmarkTimingHistogramDirect-16 29876860 39.33 ns/op 0 B/op 0 allocs/op BenchmarkWeightedHistogram-16 49227044 24.13 ns/op 0 B/op 0 allocs/op BenchmarkHistogram-16 41063907 28.82 ns/op 0 B/op 0 allocs/op PASS ok k8s.io/component-base/metrics/prometheusextension 5.432s (base) mspreitz@mjs12 prometheusextension % go test -benchmem -run=^$ -bench Histogram . goos: darwin goarch: amd64 pkg: k8s.io/component-base/metrics/prometheusextension cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz BenchmarkTimingHistogramLayered-16 22483816 51.72 ns/op 0 B/op 0 allocs/op BenchmarkTimingHistogramDirect-16 29697291 39.39 ns/op 0 B/op 0 allocs/op BenchmarkWeightedHistogram-16 48919845 24.03 ns/op 0 B/op 0 allocs/op BenchmarkHistogram-16 41153044 29.26 ns/op 0 B/op 0 allocs/op PASS ok k8s.io/component-base/metrics/prometheusextension 5.044s Remove layered implementation of TimingHistogram

MikeSpreitzer · 2022-04-28T21:37:20Z

And the force-push to b4a40cd adds the # HELP tweak to WeightedHistograms too.

wojtek-t · 2022-04-29T08:27:24Z

This LGTM. Based on the comments above from @dgrisonnet I'm going to approve it, but I will leave stamping as lgtm to @dgrisonnet or @logicalhan

@dgrisonnet - would you mind taking a look?

/approve

k8s-ci-robot · 2022-04-29T08:27:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MikeSpreitzer, wojtek-t

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~hack/OWNERS~~ [wojtek-t]
~~staging/src/k8s.io/component-base/metrics/OWNERS~~ [wojtek-t]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dgrisonnet · 2022-04-29T14:24:03Z

Great job @MikeSpreitzer!

/lgtm

Note that this is only an extension of the proto spec. Both generators and consumers of the protobuf still need changes to make use of these changes. Gauge histograms measure current distributions. For one, they are inspired by the GaugeHistogram type introducted by OpenMetrics, see https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#gaugehistogram They are also handled in the same way as OpenMetrics does it, by using a new MetricType enum field GAUGE_HISTOGRAM, but not changing anything else, i.e. for both regular and gauge histograms, the same Histogram message type is used. The other reason why we need gauge histograms comes from PromQL: If you `rate` a histogram (which is possible with the new sparse histograms as 1st class data type), the result is a gauge histogram. A rate'd histogram can be created by a recording rule and then stored in the TSDB. From there, it can be exposed by federation, so we need to be able to represent it in the exposition format. Float histograms are histograms where all counts (count of observations, counts in each bucket, zero bucket count) are floating point numbers rather than integer numbers. They are rarely needed for direct instrumentation. Use cases are weighted histograms or timing histograms, see kubernetes/kubernetes#109277 for a real-world example. However, float histograms happen all the time as results of PromQL expressions. Following the same line of argument as above, those float histograms can end up in the TSDB via recording rules, which means they can be exposed via federation. Note that float histograms are implicitly supported by the original Prometheus text format, as this format simply uses floating point numbers for all sample values. OpenMetrics has avoided this ambiguity and has specified integers for bucket counts and the count of observations in a histogram, which means it needs to be extended to support float histograms, similar to how this commit extends the original Prometheus protobuf format. Signed-off-by: beorn7 <[email protected]>

brandond · 2022-06-16T07:02:13Z

@MikeSpreitzer the TBD user-facing change from the PR description has gone out in the 1.25 alpha release notes. Can this be corrected for future releases?

MikeSpreitzer · 2022-06-16T19:15:48Z

I am not sure I understand the question. This particular PR has no release note. It is part of a larger campaign that does have release notes and that I hope to finish in 1.25.

brandond · 2022-06-16T19:19:07Z

The bit of text immediately after the Does this PR introduce a user-facing change? question in the PR template is scraped verbatim into the release notes. Because it just says TBD, that's the release note entry for this PR.

https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.25.md#:~:text=tbd

MikeSpreitzer · 2022-06-17T05:53:49Z

Oh, I see. Yes, that is not what I want. How can I fix that?

brandond · 2022-06-17T08:14:18Z

I'm not sure if changing it after the fact will effect release notes going forward, or if it'll take someone from sig-release fix?

k8s-ci-robot requested a review from wojtek-t April 4, 2022 07:04

MikeSpreitzer mentioned this pull request Apr 4, 2022

Start drafting timing histogram #109094

Closed

wojtek-t reviewed Apr 4, 2022

View reviewed changes

MikeSpreitzer mentioned this pull request Apr 4, 2022

Remove prometheus dependencies from k/k codebase #89267

Open

7 tasks

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 4, 2022

MikeSpreitzer mentioned this pull request Apr 4, 2022

Disable watermarks #108735

Closed

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Apr 5, 2022

logicalhan reviewed Apr 7, 2022

View reviewed changes

k8s-ci-robot assigned dgrisonnet and logicalhan Apr 7, 2022

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 7, 2022

kubernetes deleted a comment from logicalhan Apr 8, 2022

MikeSpreitzer force-pushed the add-weighted-histogram branch from 301a655 to 5f8b53e Compare April 28, 2022 21:26

MikeSpreitzer force-pushed the add-weighted-histogram branch from 5f8b53e to b4a40cd Compare April 28, 2022 21:36

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 29, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 29, 2022

This was referenced Apr 29, 2022

Wrap weighted histograms #109729

Merged

Replace sample-and-watermark histograms with use of TimingHistograms #109742

Closed

k8s-ci-robot merged commit cbb164e into kubernetes:master May 4, 2022

k8s-ci-robot added this to the v1.25 milestone May 4, 2022

MikeSpreitzer deleted the add-weighted-histogram branch May 4, 2022 04:16

github-actions bot mentioned this pull request May 11, 2022

Week Ending May 8, 2022 dev-obs/actus#434

Open

beorn7 mentioned this pull request May 20, 2022

OpenMetrics SparseHistogram/NativeHistograms prometheus/OpenMetrics#237

Open

beorn7 mentioned this pull request Jun 14, 2022

Add float histograms and gauge histograms to proto spec prometheus/client_model#58

Merged

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 21, 2022

		"github.com/prometheus/client_golang/prometheus"

		dto "github.com/prometheus/client_model/go"

Start drafting weighted and timing histograms #109277

Start drafting weighted and timing histograms #109277

Conversation

MikeSpreitzer commented Apr 4, 2022 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serathius commented Apr 4, 2022

MikeSpreitzer commented Apr 4, 2022

cici37 commented Apr 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeSpreitzer Apr 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logicalhan commented Apr 7, 2022

dgrisonnet commented Apr 26, 2022

MikeSpreitzer commented Apr 27, 2022 • edited Loading

MikeSpreitzer commented Apr 27, 2022

dgrisonnet commented Apr 28, 2022

MikeSpreitzer commented Apr 28, 2022

MikeSpreitzer commented Apr 28, 2022

wojtek-t commented Apr 29, 2022

k8s-ci-robot commented Apr 29, 2022

dgrisonnet commented Apr 29, 2022

brandond commented Jun 16, 2022

MikeSpreitzer commented Jun 16, 2022

brandond commented Jun 16, 2022 • edited Loading

MikeSpreitzer commented Jun 17, 2022 • edited Loading

brandond commented Jun 17, 2022

MikeSpreitzer commented Apr 4, 2022 •

edited

Loading

MikeSpreitzer Apr 10, 2022 •

edited

Loading

MikeSpreitzer commented Apr 27, 2022 •

edited

Loading

brandond commented Jun 16, 2022 •

edited

Loading

MikeSpreitzer commented Jun 17, 2022 •

edited

Loading