Add unit test for sample-and-watermark histograms #97753

MikeSpreitzer · 2021-01-06T04:35:26Z

What type of PR is this?

/kind cleanup
/kind regression

What this PR does / why we need it:
This PR adds unit tests for sample-and-watermark histograms. This is good for two reasons. One is helping to investigate the possible compiler bug in #97685 . The other is to satisfy #96543 .

Which issue(s) this PR fixes:

Fixes #96543

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

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

MikeSpreitzer · 2021-01-06T04:35:58Z

@kubernetes/sig-api-machinery-bugs

MikeSpreitzer · 2021-01-06T04:38:31Z

/cc @lavalamp
@yue9944882
@tkashem

MikeSpreitzer · 2021-01-06T04:39:25Z

@erikwilson
@brandond

MikeSpreitzer · 2021-01-06T05:07:42Z

@brandond you can exercise this unit test as follows.

cd staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics
go test

For details on the calculations, make it go test . -v.

MikeSpreitzer · 2021-01-06T08:40:43Z

@adtac
@wojtek-t

Add info message that explains what warnings are deliberate and expected.

MikeSpreitzer · 2021-01-06T18:11:23Z

The force-push to 194c22f fixes the traditional bazel oversight.

caesarxuchao · 2021-01-12T21:08:43Z

/triage accepted

lavalamp · 2021-01-15T23:17:02Z

I think in the comment of mine that apparently inspired this, I was hoping for a tabular test of just the quantize function. This is testing a lot more than that and I'm finding it very difficult to follow.

lavalamp · 2021-01-16T00:16:10Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/sample_and_watermark_test.go

+	klog.Infof("Expect about %v warnings about time going backwards; this is fake time deliberately misbehaving.", (numIterations*ddtOffset)/ddtRange)
+	t.Logf("t0=%s", t0)
+	for i := 0; i < numIterations; i++ {
+		ddt := time.Microsecond * time.Duration(rand.Intn(ddtRange)-ddtOffset)


I'm sorry, I've been staring at this for a while now and I don't really get what exactly is being tested or why it's being tested like this :( I'm really sorry to give feedback this vague but I'm not sure what would help here.

I added a comment explaining the new test func.
I prefer behavior tests to checksum tests. This behavioral test exercises the quantize function, so should cover the original ask.

logicalhan · 2021-01-16T00:22:49Z

/cc @YoyinZyc

YoyinZyc · 2021-01-19T19:54:34Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/sample_and_watermark_test.go

+		ddt := time.Microsecond * time.Duration(rand.Intn(ddtRange)-ddtOffset)
+		t1 = t1.Add(ddt)
+		diff := t1.Sub(t0)
+		if diff > dt {


I am so sorry I don't really get the logic behind this code. Can you explain more about the relationship between clock and the histogram count? Also how do you calculate the expectedCount?

Added/expanded comments, hopefully it is clearer now.

YoyinZyc · 2021-01-19T20:02:57Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/sample_and_watermark_test.go

+	t0 := time.Now()
+	clk := clock.NewFakePassiveClock(t0)
+	buckets := []float64{0, 1}
+	const samplingPeriod = time.Millisecond


Put samplingPeriod into above const block?

YoyinZyc · 2021-01-19T20:03:01Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/sample_and_watermark_test.go

+	for _, reg := range regs {
+		legacyregistry.MustRegister(reg)
+	}
+	dt := 2 * samplingPeriod


what is dt? Why it is 2 samplingPeriod?

Added comment explaining what dt is. Changed my mind about initialization; the original was to avoid the case of a net negative time difference, and I decided to not try to avoid that.

YoyinZyc · 2021-01-19T20:03:25Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/sample_and_watermark_test.go

+			dt = diff
+		}
+		clk.SetTime(t1)
+		saw.Set(1)


Do you need to test other timeObserver operation like Add or SetX1?

Additional tests could be created for those. This one is explicitly and exclusively focused on the question of when samples are taken.

YoyinZyc · 2021-02-01T19:19:47Z

It is much clearer with the comments. I am good to go. Thanks for adding the tests. :)
/lgtm

lavalamp · 2021-02-01T22:28:48Z

/approve
/sig api-machinery

k8s-ci-robot · 2021-02-01T22:29:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, MikeSpreitzer

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:

~~staging/src/k8s.io/apiserver/pkg/util/flowcontrol/OWNERS~~ [MikeSpreitzer,lavalamp]

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

MikeSpreitzer · 2021-02-02T06:06:11Z

/sig api-machinery

MikeSpreitzer · 2021-02-02T06:08:07Z

@kubernetes/sig-api-machinery-bugs

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. kind/bug Categorizes issue or PR as related to a bug. area/apiserver area/dependency Issues or PRs related to dependency changes labels Jan 6, 2021

k8s-ci-robot requested review from deads2k, liggitt and a team January 6, 2021 04:36

k8s-ci-robot requested a review from lavalamp January 6, 2021 04:38

MikeSpreitzer force-pushed the test-saw branch from b65e1f3 to 5c92ff5 Compare January 6, 2021 05:04

MikeSpreitzer force-pushed the test-saw branch from 5c92ff5 to 546cee3 Compare January 6, 2021 05:32

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 6, 2021

MikeSpreitzer mentioned this pull request Jan 6, 2021

performance regression on ARM: 1.20 apiserver has high cpu usage #97685

Closed

Add unit test for sample-and-watermark histograms

5fe48be

MikeSpreitzer force-pushed the test-saw branch from 546cee3 to 5fe48be Compare January 6, 2021 08:22

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 6, 2021

Explain warnings in new test

194c22f

Add info message that explains what warnings are deliberate and expected.

MikeSpreitzer force-pushed the test-saw branch from 77e9507 to 194c22f Compare January 6, 2021 18:11

BenTheElder removed the request for review from a team January 6, 2021 18:58

This was referenced Jan 7, 2021

Pass by reference to sampleAndWaterMarkObserverGenerator::quantize #97791

Closed

pass-by-reference for sampleAndWaterMarkObserverGenerator::quantize #97854

Closed

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 Jan 12, 2021

lavalamp reviewed Jan 16, 2021

View reviewed changes

k8s-ci-robot requested a review from YoyinZyc January 16, 2021 00:22

Added comment explaining the new unit test

42f8d4e

YoyinZyc reviewed Jan 19, 2021

View reviewed changes

Tweak up TestSampler in response to review

736a87a

k8s-ci-robot assigned YoyinZyc Feb 1, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 1, 2021

BenTheElder removed the do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Feb 2, 2021

k8s-ci-robot merged commit b36afa5 into kubernetes:master Feb 2, 2021

k8s-ci-robot added this to the v1.21 milestone Feb 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit test for sample-and-watermark histograms #97753

Add unit test for sample-and-watermark histograms #97753

MikeSpreitzer commented Jan 6, 2021

MikeSpreitzer commented Jan 6, 2021

MikeSpreitzer commented Jan 6, 2021

MikeSpreitzer commented Jan 6, 2021

MikeSpreitzer commented Jan 6, 2021 •

edited

MikeSpreitzer commented Jan 6, 2021

MikeSpreitzer commented Jan 6, 2021

caesarxuchao commented Jan 12, 2021

lavalamp commented Jan 15, 2021

lavalamp Jan 16, 2021

MikeSpreitzer Jan 18, 2021

logicalhan commented Jan 16, 2021

YoyinZyc Jan 19, 2021

MikeSpreitzer Jan 25, 2021

YoyinZyc Jan 19, 2021

MikeSpreitzer Jan 25, 2021

YoyinZyc Jan 19, 2021

MikeSpreitzer Jan 25, 2021

YoyinZyc Jan 19, 2021

MikeSpreitzer Jan 25, 2021

YoyinZyc commented Feb 1, 2021

lavalamp commented Feb 1, 2021

k8s-ci-robot commented Feb 1, 2021

MikeSpreitzer commented Feb 2, 2021

MikeSpreitzer commented Feb 2, 2021

Add unit test for sample-and-watermark histograms #97753

Add unit test for sample-and-watermark histograms #97753

Conversation

MikeSpreitzer commented Jan 6, 2021

MikeSpreitzer commented Jan 6, 2021

MikeSpreitzer commented Jan 6, 2021

MikeSpreitzer commented Jan 6, 2021

MikeSpreitzer commented Jan 6, 2021 • edited

MikeSpreitzer commented Jan 6, 2021

MikeSpreitzer commented Jan 6, 2021

caesarxuchao commented Jan 12, 2021

lavalamp commented Jan 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logicalhan commented Jan 16, 2021

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

YoyinZyc commented Feb 1, 2021

lavalamp commented Feb 1, 2021

k8s-ci-robot commented Feb 1, 2021

MikeSpreitzer commented Feb 2, 2021

MikeSpreitzer commented Feb 2, 2021

MikeSpreitzer commented Jan 6, 2021 •

edited