Skip to content

Commit c78ecb9

Browse files
craig[bot]miraradevaherkolategan
committed
142830: storeliveness: add documentation r=miraradeva a=miraradeva This commit adds a `doc.go` file in the storeliveness package to document the main functionality and interactions with other components. Fixes: #126198 Release note: None 143061: microbench-ci: compare only outer run when insignificant r=DarrylWong a=herkolategan Previously, during the comparison step all runs were compared. Since the last run determines if the previous runs had significant changes, we now first compare only the last outer run of each benchmark. If the last run had significant changes, we then compare all runs to produce the final assessment. This prevents having a compare summary on CI that shows a regressions, when in fact only one run possibly had a regression, followed by an insignificant change in the last run. i.e., Did the last run (3rd out of 3 runs) contain a regression / improvement? *yes*: then implicitly all previous runs contained a regression or improvement, and should be included in the result summary. *no*: then it's possible a previous run did regress, but we're disregarding the result in any case (due to requiring consecutive regressions in totality); hence only present the last insignificant run in the summary, to avoid confusion. Epic: None Release note: None Co-authored-by: Mira Radeva <[email protected]> Co-authored-by: Herko Lategan <[email protected]>
3 parents 9dbc353 + a7c304f + 9b48cc1 commit c78ecb9

File tree

6 files changed

+301
-102
lines changed

6 files changed

+301
-102
lines changed

‎pkg/cmd/microbench-ci/compare.go

+15-2
Original file line numberDiff line numberDiff line change
@@ -121,14 +121,27 @@ func (b *Benchmark) compare(lines int) (*CompareResult, error) {
121121
return &compareResult, nil
122122
}
123123

124-
// compareBenchmarks compares the metrics of all benchmarks between two revisions.
124+
// compareBenchmarks compares the metrics of all benchmarks between two
125+
// revisions. It first compares only the last outer run of each benchmark. If
126+
// the last run had significant changes, it compares the metrics of all runs.
127+
// This is because the last run would only have completed with significant
128+
// changes if all the previous runs had them as well, and then we want to
129+
// include it in the final assessment. In contrast if the last run had no
130+
// significant changes, it is possible that the previous runs had significant
131+
// changes, and we don't want to include them in the final assessment.
125132
func (b Benchmarks) compareBenchmarks() (CompareResults, error) {
126133
compareResults := make(CompareResults, 0, len(b))
127134
for _, benchmark := range b {
128-
compareResult, err := benchmark.compare(0)
135+
compareResult, err := benchmark.compare(benchmark.Count)
129136
if err != nil {
130137
return nil, err
131138
}
139+
if compareResult.top() != NoChange {
140+
compareResult, err = benchmark.compare(0)
141+
if err != nil {
142+
return nil, err
143+
}
144+
}
132145
compareResults = append(compareResults, compareResult)
133146
}
134147
return compareResults, nil

‎pkg/cmd/microbench-ci/run.go

+5-6
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,7 @@ func (b *Benchmark) run() error {
103103
// iterations. Retries are run once a benchmark has regressed or improved,
104104
// on the first try, to ensure that the change is not a fluke.
105105
if currentStatus == NoChange {
106+
status = currentStatus
106107
break
107108
}
108109

@@ -122,12 +123,10 @@ func (b *Benchmark) run() error {
122123

123124
// Write change marker file if the benchmark changed.
124125
if status != NoChange {
125-
for _, revision := range []Revision{New, Old} {
126-
marker := strings.ToUpper(status.String())
127-
err := os.WriteFile(path.Join(suite.artifactsDir(revision), "."+marker), nil, 0644)
128-
if err != nil {
129-
return err
130-
}
126+
marker := strings.ToUpper(status.String())
127+
err := os.WriteFile(path.Join(suite.artifactsDir(New), "."+marker), nil, 0644)
128+
if err != nil {
129+
return err
131130
}
132131
}
133132

‎pkg/cmd/microbench-ci/testdata/regression.txt

-1
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,6 @@ tree
147147

148148
/abcdef123
149149
/abcdef123/artifacts
150-
/abcdef123/artifacts/.REGRESSED
151150
/abcdef123/artifacts/cleaned_Sysbench_SQL_3node_oltp_read_write.log
152151
/abcdef123/artifacts/cpu_Sysbench_SQL_3node_oltp_read_write_merged.prof
153152
/abcdef123/artifacts/memory_Sysbench_SQL_3node_oltp_read_write_merged.prof

‎pkg/cmd/microbench-ci/testdata/summary.txt

+21-93
Original file line numberDiff line numberDiff line change
@@ -72,8 +72,8 @@ run group=1
7272

7373
| Metric | Old Commit | New Commit | Delta | Note |
7474
|-----------------------------|----------------|----------------|------------|--------------|
75-
| ⚪ **sec/op** | 9.852m ±0% | 9.880m ±1% | ~ | p=0.084 n=20 |
76-
| ⚪ **allocs/op** | 10.38k ±0% | 10.38k ±0% | ~ | p=1.000 n=20 |
75+
| ⚪ **sec/op** | 9.852m ±0% | 9.852m ±0% | ~ | p=1.000 n=10 |
76+
| ⚪ **allocs/op** | 10.38k ±1% | 10.38k ±1% | ~ | p=1.000 n=10 |
7777

7878
<details><summary>Reproduce</summary>
7979

@@ -127,7 +127,6 @@ tree
127127

128128
/abcdef123
129129
/abcdef123/artifacts
130-
/abcdef123/artifacts/.REGRESSED
131130
/abcdef123/artifacts/cleaned_Sysbench_SQL_3node_oltp_read_write.log
132131
/abcdef123/artifacts/cpu_Sysbench_SQL_3node_oltp_read_write_merged.prof
133132
/abcdef123/artifacts/memory_Sysbench_SQL_3node_oltp_read_write_merged.prof
@@ -136,7 +135,6 @@ tree
136135
/github-summary.md
137136
/qwerty456
138137
/qwerty456/artifacts
139-
/qwerty456/artifacts/.REGRESSED
140138
/qwerty456/artifacts/cleaned_Sysbench_SQL_3node_oltp_read_write.log
141139
/qwerty456/artifacts/cpu_Sysbench_SQL_3node_oltp_read_write_merged.prof
142140
/qwerty456/artifacts/memory_Sysbench_SQL_3node_oltp_read_write_merged.prof
@@ -161,32 +159,22 @@ json
161159
"Metric": "B/op",
162160
"Summary": {
163161
"Center": 2367667,
164-
"Lo": 2364281,
165-
"Hi": 2369187,
166-
"Confidence": 0.95861,
162+
"Lo": 2358650,
163+
"Hi": 2370670,
164+
"Confidence": 0.97852,
167165
"Warnings": null
168166
},
169167
"Sample": {
170168
"Values": [
171169
2352326,
172-
2352326,
173-
2358650,
174170
2358650,
175171
2364281,
176-
2364281,
177-
2365463,
178172
2365463,
179173
2367582,
180-
2367582,
181-
2367752,
182174
2367752,
183175
2368213,
184-
2368213,
185176
2369187,
186-
2369187,
187-
2370670,
188177
2370670,
189-
2375306,
190178
2375306
191179
],
192180
"Thresholds": {
@@ -199,32 +187,22 @@ json
199187
"Metric": "allocs/op",
200188
"Summary": {
201189
"Center": 10378.50000,
202-
"Lo": 10361,
203-
"Hi": 10392,
204-
"Confidence": 0.95861,
190+
"Lo": 10287,
191+
"Hi": 10398,
192+
"Confidence": 0.97852,
205193
"Warnings": null
206194
},
207195
"Sample": {
208196
"Values": [
209197
10246,
210-
10246,
211-
10287,
212198
10287,
213199
10361,
214-
10361,
215-
10377,
216200
10377,
217201
10378,
218-
10378,
219202
10379,
220-
10379,
221-
10386,
222203
10386,
223204
10392,
224-
10392,
225-
10398,
226205
10398,
227-
10411,
228206
10411
229207
],
230208
"Thresholds": {
@@ -236,10 +214,10 @@ json
236214
{
237215
"Metric": "sec/op",
238216
"Summary": {
239-
"Center": 0.00988,
217+
"Center": 0.00985,
240218
"Lo": 0.00985,
241-
"Hi": 0.00995,
242-
"Confidence": 0.95861,
219+
"Hi": 0.00985,
220+
"Confidence": 0.97852,
243221
"Warnings": null
244222
},
245223
"Sample": {
@@ -253,17 +231,7 @@ json
253231
0.00985,
254232
0.00985,
255233
0.00985,
256-
0.00985,
257-
0.00991,
258-
0.00993,
259-
0.00995,
260-
0.00995,
261-
0.00995,
262-
0.00995,
263-
0.00997,
264-
0.00998,
265-
0.00998,
266-
0.01000
234+
0.00985
267235
],
268236
"Thresholds": {
269237
"CompareAlpha": 0.05000
@@ -277,32 +245,22 @@ json
277245
"Metric": "B/op",
278246
"Summary": {
279247
"Center": 2367667,
280-
"Lo": 2364281,
281-
"Hi": 2369187,
282-
"Confidence": 0.95861,
248+
"Lo": 2358650,
249+
"Hi": 2370670,
250+
"Confidence": 0.97852,
283251
"Warnings": null
284252
},
285253
"Sample": {
286254
"Values": [
287255
2352326,
288-
2352326,
289-
2358650,
290256
2358650,
291257
2364281,
292-
2364281,
293258
2365463,
294-
2365463,
295-
2367582,
296259
2367582,
297260
2367752,
298-
2367752,
299261
2368213,
300-
2368213,
301-
2369187,
302262
2369187,
303263
2370670,
304-
2370670,
305-
2375306,
306264
2375306
307265
],
308266
"Thresholds": {
@@ -315,32 +273,22 @@ json
315273
"Metric": "allocs/op",
316274
"Summary": {
317275
"Center": 10378.50000,
318-
"Lo": 10361,
319-
"Hi": 10392,
320-
"Confidence": 0.95861,
276+
"Lo": 10287,
277+
"Hi": 10398,
278+
"Confidence": 0.97852,
321279
"Warnings": null
322280
},
323281
"Sample": {
324282
"Values": [
325-
10246,
326283
10246,
327284
10287,
328-
10287,
329-
10361,
330285
10361,
331286
10377,
332-
10377,
333287
10378,
334-
10378,
335-
10379,
336288
10379,
337289
10386,
338-
10386,
339-
10392,
340290
10392,
341291
10398,
342-
10398,
343-
10411,
344292
10411
345293
],
346294
"Thresholds": {
@@ -355,21 +303,11 @@ json
355303
"Center": 0,
356304
"Lo": 0,
357305
"Hi": 0,
358-
"Confidence": 0.95861,
306+
"Confidence": 0.97852,
359307
"Warnings": null
360308
},
361309
"Sample": {
362310
"Values": [
363-
0,
364-
0,
365-
0,
366-
0,
367-
0,
368-
0,
369-
0,
370-
0,
371-
0,
372-
0,
373311
0,
374312
0,
375313
0,
@@ -393,17 +331,11 @@ json
393331
"Center": 0.00985,
394332
"Lo": 0.00985,
395333
"Hi": 0.00985,
396-
"Confidence": 0.95861,
334+
"Confidence": 0.97852,
397335
"Warnings": null
398336
},
399337
"Sample": {
400338
"Values": [
401-
0.00981,
402-
0.00985,
403-
0.00985,
404-
0.00985,
405-
0.00985,
406-
0.00985,
407339
0.00985,
408340
0.00985,
409341
0.00985,
@@ -413,11 +345,7 @@ json
413345
0.00985,
414346
0.00985,
415347
0.00985,
416-
0.00987,
417-
0.00988,
418-
0.00990,
419-
0.00993,
420-
0.00998
348+
0.00985
421349
],
422350
"Thresholds": {
423351
"CompareAlpha": 0.05000

‎pkg/kv/kvserver/storeliveness/BUILD.bazel

+1
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ go_library(
44
name = "storeliveness",
55
srcs = [
66
"config.go",
7+
"doc.go",
78
"fabric.go",
89
"metrics.go",
910
"node_manager.go",

0 commit comments

Comments
 (0)