[telemetry] godev/cmd/worker: add readCount method and unit test
0 views
Skip to first unread message
Hongxiang Jiang (Gerrit)
3:40 PM (6 hours ago) 3:40 PM
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Hongxiang Jiang has uploaded the change for review![]( "Open in Gerrit")
Commit message
godev/cmd/worker: add readCount method and unit test
Change-Id: I252684bb682f7da7557bcd8c6a0d1824285ad3da
Change diff
diff --git a/godev/cmd/worker/main.go b/godev/cmd/worker/main.go
index 31951a3..4506ffa 100644
--- a/godev/cmd/worker/main.go
+++ b/godev/cmd/worker/main.go
@@ -413,51 +413,74 @@
result := make(data)
for _, r := range reports {
for _, p := range r.Programs {
- writeCount(result, r.Week, p.Program, versionCounter, p.Version, r.X, 1)
- writeCount(result, r.Week, p.Program, goosCounter, p.GOOS, r.X, 1)
- writeCount(result, r.Week, p.Program, goarchCounter, p.GOARCH, r.X, 1)
- writeCount(result, r.Week, p.Program, goversionCounter, p.GoVersion, r.X, 1)
+ result.writeCount(r.Week, p.Program, versionCounter, p.Version, r.X, 1)
+ result.writeCount(r.Week, p.Program, goosCounter, p.GOOS, r.X, 1)
+ result.writeCount(r.Week, p.Program, goarchCounter, p.GOARCH, r.X, 1)
+ result.writeCount(r.Week, p.Program, goversionCounter, p.GoVersion, r.X, 1)
for c, value := range p.Counters {
name, _ := splitCounterName(c)
- writeCount(result, r.Week, p.Program, name, c, r.X, value)
+ result.writeCount(r.Week, p.Program, name, c, r.X, value)
}
}
}
return result
}
+// readCount read the count value based on the input keys.
+// Return nil if any key does not exist.
+func (d data) readCount(week, program, prefix, counter string, x float64) *int64 {
+ wk := weekKey{week}
+ if _, ok := d[wk]; !ok {
+ return nil
+ }
+ pk := programKey{program}
+ if _, ok := d[wk][pk]; !ok {
+ return nil
+ }
+ gk := graphKey{prefix}
+ if _, ok := d[wk][pk][gk]; !ok {
+ return nil
+ }
+ ck := counterKey{counter}
+ if _, ok := d[wk][pk][gk][ck]; !ok {
+ return nil
+ }
+ value := d[wk][pk][gk][ck][xKey{x}]
+ return &value
+}
+
// writeCount writes the counter values to the result. When a report contains
// multiple program reports for the same program, the value of the counters
// in that report are summed together.
-func writeCount(result data, week, program, prefix, counter string, x float64, value int64) {
+func (d data) writeCount(week, program, prefix, counter string, x float64, value int64) {
wk := weekKey{week}
- if _, ok := result[wk]; !ok {
- result[wk] = make(map[programKey]map[graphKey]map[counterKey]map[xKey]int64)
+ if _, ok := d[wk]; !ok {
+ d[wk] = make(map[programKey]map[graphKey]map[counterKey]map[xKey]int64)
}
pk := programKey{program}
- if _, ok := result[wk][pk]; !ok {
- result[wk][pk] = make(map[graphKey]map[counterKey]map[xKey]int64)
+ if _, ok := d[wk][pk]; !ok {
+ d[wk][pk] = make(map[graphKey]map[counterKey]map[xKey]int64)
}
gk := graphKey{prefix}
- if _, ok := result[wk][pk][gk]; !ok {
- result[wk][pk][gk] = make(map[counterKey]map[xKey]int64)
+ if _, ok := d[wk][pk][gk]; !ok {
+ d[wk][pk][gk] = make(map[counterKey]map[xKey]int64)
}
// TODO(hyangah): let caller pass the normalized counter name.
counter = normalizeCounterName(prefix, counter)
ck := counterKey{counter}
- if _, ok := result[wk][pk][gk][ck]; !ok {
- result[wk][pk][gk][ck] = make(map[xKey]int64)
+ if _, ok := d[wk][pk][gk][ck]; !ok {
+ d[wk][pk][gk][ck] = make(map[xKey]int64)
}
xk := xKey{x}
- result[wk][pk][gk][ck][xk] += value
+ d[wk][pk][gk][ck][xk] += value
// record the total for all counters with the prefix
// as the bucket name.
if prefix != counter {
ck = counterKey{prefix}
- if _, ok := result[wk][pk][gk][ck]; !ok {
- result[wk][pk][gk][ck] = make(map[xKey]int64)
+ if _, ok := d[wk][pk][gk][ck]; !ok {
+ d[wk][pk][gk][ck] = make(map[xKey]int64)
}
- result[wk][pk][gk][ck][xk] += value
+ d[wk][pk][gk][ck][xk] += value
}
}
diff --git a/godev/cmd/worker/main_test.go b/godev/cmd/worker/main_test.go
index 16e4662..ad98a62 100644
--- a/godev/cmd/worker/main_test.go
+++ b/godev/cmd/worker/main_test.go
@@ -559,3 +559,67 @@
})
}
}
+
+func TestWriteCount(t *testing.T) {
+ type keyValue struct {
+ week, program, prefix, counter string
+ x float64
+ value int64
+ }
+ testcases := []struct {
+ name string
+ inputs []keyValue
+ wants []keyValue
+ }{
+ {
+ name: "counter w and w/o version should have value",
+ inputs: []keyValue{
+ {"2987-07-01", "golang.org/x/tools/gopls", "Version", "v0.15.3", 0.00009, 1},
+ },
+ wants: []keyValue{
+ {"2987-07-01", "golang.org/x/tools/gopls", "Version", "Version:v0.15", 0.00009, 1},
+ {"2987-07-01", "golang.org/x/tools/gopls", "Version", "Version", 0.00009, 1},
+ },
+ },
+ {
+ name: "only one count with same prefix and counter",
+ inputs: []keyValue{
+ {"2987-06-30", "cmd/go", "go/invocations", "go/invocations", 0.86995, 84},
+ },
+ wants: []keyValue{
+ {"2987-06-30", "cmd/go", "go/invocations", "go/invocations", 0.86995, 84},
+ },
+ },
+ {
+ name: "sum together when calling multiple times",
+ inputs: []keyValue{
+ {"2987-06-30", "golang.org/x/tools/gopls", "GOOS", "windows", 0.86018, 1},
+ {"2987-06-30", "golang.org/x/tools/gopls", "GOOS", "windows", 0.86018, 2},
+ {"2987-06-30", "golang.org/x/tools/gopls", "GOOS", "windows", 0.86018, 3},
+ },
+ wants: []keyValue{
+ {"2987-06-30", "golang.org/x/tools/gopls", "GOOS", "GOOS:windows", 0.86018, 6},
+ {"2987-06-30", "golang.org/x/tools/gopls", "GOOS", "GOOS", 0.86018, 6},
+ },
+ },
+ }
+
+ for _, tc := range testcases {
+ t.Run(tc.name, func(t *testing.T) {
+ d := make(data)
+ for _, input := range tc.inputs {
+ d.writeCount(input.week, input.program, input.prefix, input.counter, input.x, input.value)
+ }
+
+ for _, want := range tc.wants {
+ got := d.readCount(want.week, want.program, want.prefix, want.counter, want.x)
+ if got == nil {
+ t.Errorf("d[%q][%q][%q][%q][%v] should not be nil", want.week, want.program, want.prefix, want.counter, want.x)
+ }
+ if got != nil && want.value != *got {
+ t.Errorf("d[%q][%q][%q][%q][%v] = %v, want %v", want.week, want.program, want.prefix, want.counter, want.x, *got, want.value)
+ }
+ }
+ })
+ }
+}
Change information
Files:
- M godev/cmd/worker/main.go
- M godev/cmd/worker/main_test.go
Change size: M
Delta: 2 files changed, 105 insertions(+), 18 deletions(-)
Related details
Attention set is empty
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
4:06 PM (5 hours ago) 4:06 PM
to goph...@pubsubhelper.golang.org, Hyang-Ah Hana Kim, Robert Findley, golang-co...@googlegroups.com
Attention needed from Hyang-Ah Hana Kim and Robert Findley
Hongxiang Jiang voted![]( "Open in Gerrit")
| Commit-Queue | +1 |
| Run-TryBot | +1 |
Related details
Attention is currently required from:
- Hyang-Ah Hana Kim
- Robert Findley
Submit Requirements:
Code-Review
Legacy-TryBots-Pass
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
4:09 PM (5 hours ago) 4:09 PM
to goph...@pubsubhelper.golang.org, Gopher Robot, Go LUCI, Hyang-Ah Hana Kim, Robert Findley, golang-co...@googlegroups.com
Attention needed from Hyang-Ah Hana Kim and Robert Findley
Hongxiang Jiang added 1 comment![]( "Open in Gerrit")
File godev/cmd/worker/main.go
Line 476, Patchset 2 (Latest):
// record the total for all counters with the prefix
// as the bucket name.
if prefix != counter {Hongxiang Jiang . unresolved
Question: If the `prefix != counter`, why we keep this information in two places?
Say the input is
`"2987-06-30", "golang.org/x/tools/gopls", "GOOS", "windows", 0.86018, 1`
We will have two entry in the map containing this `1`:
```
"2987-06-30", "golang.org/x/tools/gopls", "GOOS", "GOOS:windows", 0.86018 -> 1
"2987-06-30", "golang.org/x/tools/gopls", "GOOS", "GOOS", 0.86018 -> 1
```
Related details
Attention is currently required from:
- Hyang-Ah Hana Kim
- Robert Findley
Submit Requirements:
Code-Review
Legacy-TryBots-Pass
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
4:27 PM (5 hours ago) 4:27 PM
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Hongxiang Jiang, Hyang-Ah Hana Kim and Robert Findley
Hongxiang Jiang uploaded new patchset![]( "Open in Gerrit")
Hongxiang Jiang uploaded patch set #3 to this change.
Following approvals got outdated and were removed:
- Legacy-TryBots-Pass: TryBot-Result+1 by Gopher Robot, Run-TryBot+1 by Hongxiang Jiang
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
Attention is currently required from:
- Hongxiang Jiang
- Hyang-Ah Hana Kim
- Robert Findley
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
4:27 PM (5 hours ago) 4:27 PM
to goph...@pubsubhelper.golang.org, Go LUCI, Gopher Robot, Hyang-Ah Hana Kim, Robert Findley, golang-co...@googlegroups.com
Attention needed from Hyang-Ah Hana Kim and Robert Findley
Hongxiang Jiang voted![]( "Open in Gerrit")
Attention is currently required from:
- Hyang-Ah Hana Kim
- Robert Findley
Submit Requirements:
Code-Review
Legacy-TryBots-Pass
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hyang-Ah Hana Kim (Gerrit)
5:39 PM (4 hours ago) 5:39 PM
to Hongxiang Jiang, goph...@pubsubhelper.golang.org, Gopher Robot, Go LUCI, Hyang-Ah Hana Kim, Robert Findley, golang-co...@googlegroups.com
Attention needed from Hongxiang Jiang and Robert Findley
Hyang-Ah Hana Kim added 4 comments![]( "Open in Gerrit")
File godev/cmd/worker/main.go
Line 429, Patchset 3 (Latest):
// readCount read the count value based on the input keys.Hyang-Ah Hana Kim . unresolved
s/read/reads
Can you make `readCount` return `int64` or `(count int64, ok bool)` if you need to handle missing count different? Using pointer `nil` to signal error is unusual.
Line 476, Patchset 2:
// record the total for all counters with the prefix
// as the bucket name.
if prefix != counter {Hongxiang Jiang . unresolved
Question: If the `prefix != counter`, why we keep this information in two places?
Say the input is
`"2987-06-30", "golang.org/x/tools/gopls", "GOOS", "windows", 0.86018, 1`
We will have two entry in the map containing this `1`:
```
"2987-06-30", "golang.org/x/tools/gopls", "GOOS", "GOOS:windows", 0.86018 -> 1
"2987-06-30", "golang.org/x/tools/gopls", "GOOS", "GOOS", 0.86018 -> 1
```
Hyang-Ah Hana Kim
I think this is to handle the counter with chart:bucket format name. https://pkg.go.dev/golang.org/x/telemetry/counter#hdr-Counter_Naming.
For example, gopls records the following counters
`gopls/client:vscode`,
`gopls/client:vscodium`,
...
They are separate but related counters, and share the prefix (chart name) `gopls/client` but different bucket names (vscode, vscodium, ...). This code tries to compute the sum of all counters with the same prefix.
`GOOS`, `GOARCH`, ... are fake counters derived from the metadata, so I expect there will be no counter "GOOS:windows" in the original counter file. However the worker tries to handle them in the similar way of handling usual chart:bucket style counters using the same code.
So, the map contains all counters seen from the counter files and synthesized
(GOOS:windows, GOOS:linux, ...) as they are. And also one extra entry (`GOOS`) that represents the sum of {GOOS:windows, GOOS:linux, ...} counters.
File godev/cmd/worker/main_test.go
Line 575, Patchset 3 (Latest):
name: "counter w and w/o version should have value",Hyang-Ah Hana Kim . unresolved
How about "program version counter"?
Avoid `/` in the subtest name. "/" is special.
https://github.com/golang/vscode-go/issues/3286#issuecomment-2065055214
Line 598, Patchset 3 (Latest):
{"2987-06-30", "golang.org/x/tools/gopls", "GOOS", "windows", 0.86018, 3},Hyang-Ah Hana Kim . unresolved
You can also add { ..., "GOOS", "linux", ...} to the input and see if "GOOS" entry is the expected sum.
Related details
Attention is currently required from:
- Hongxiang Jiang
- Robert Findley
Submit Requirements:
Code-Review
Legacy-TryBots-Pass
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages