Skip to content

Commit f42d155

Browse files
authored
dyninst/loader: fix bug with circuit breaker stats (#43323)
### What does this PR do? Before this change we were interpretting the number of nanoseconds as the number of hits. This lead to quite incorrect logic with regards to the circuit breaker firing (it would fire too often). ### Motivation I noticed the circuit breaker tripping when it absolutely should not have been. ### Describe how you validated your changes Added a test. ### Additional Notes Relates to https://datadoghq.atlassian.net/browse/DEBUG-4202. Co-authored-by: andrew.werner <[email protected]>
1 parent d0b9f63 commit f42d155

File tree

2 files changed

+43
-7
lines changed

2 files changed

+43
-7
lines changed

pkg/dyninst/loader/loader.go

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -182,12 +182,12 @@ func (p *Program) Close() {
182182

183183
// RuntimeStats are cumulative stats aggregated throughout program lifetime.
184184
type RuntimeStats struct {
185+
// Aggregated cpu time spent in probe execution (excluding interrupt overhead).
186+
CPU time.Duration
185187
// Number of probe hits.
186188
HitCnt uint64
187189
// Number of probe hits that skipped data capture due to throttling.
188190
ThrottledCnt uint64
189-
// Aggregated cpu time spent in probe execution (excluding interrupt overhead).
190-
CPU time.Duration
191191
}
192192

193193
// RuntimeStats returns the per-core runtime stats for the program.
@@ -198,11 +198,14 @@ func (p *Program) RuntimeStats() []RuntimeStats {
198198
}
199199
entries := statsMap.Iterate()
200200
var key uint32
201-
var stats []RuntimeStats
202-
if !entries.Next(&key, &stats) {
203-
return stats
204-
}
205-
return stats
201+
var stats []stats
202+
_ = entries.Next(&key, &stats)
203+
// This is safe because these two structs have the same layout.
204+
// See TestRuntimeStatsHasSameLayoutAsStats for more details.
205+
return unsafe.Slice(
206+
(*RuntimeStats)(unsafe.Pointer(unsafe.SliceData(stats))),
207+
len(stats),
208+
)
206209
}
207210

208211
const defaultRingbufSize = 1 << 20 // 1 MiB

pkg/dyninst/loader/stats_test.go

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
// Unless explicitly stated otherwise all files in this repository are licensed
2+
// under the Apache License Version 2.0.
3+
// This product includes software developed at Datadog (https://www.datadoghq.com/).
4+
// Copyright 2016-present Datadog, Inc.
5+
6+
//go:build linux_bpf
7+
8+
package loader
9+
10+
import (
11+
"crypto/rand"
12+
"testing"
13+
"time"
14+
"unsafe"
15+
16+
"github.com/stretchr/testify/require"
17+
)
18+
19+
// Verify that the RuntimeStats struct has the same layout as the stats struct
20+
// used in C.
21+
func TestRuntimeStatsHasSameLayoutAsStats(t *testing.T) {
22+
require.Equal(t, unsafe.Sizeof(RuntimeStats{}), unsafe.Sizeof(stats{}))
23+
bytes := make([]byte, unsafe.Sizeof(stats{}))
24+
rand.Read(bytes)
25+
cStats := *(*stats)(unsafe.Pointer(&bytes[0]))
26+
runtimeStats := RuntimeStats{
27+
CPU: time.Duration(cStats.Cpu_ns),
28+
HitCnt: cStats.Hit_cnt,
29+
ThrottledCnt: cStats.Throttled_cnt,
30+
}
31+
require.Equal(t, runtimeStats, *(*RuntimeStats)(unsafe.Pointer(&cStats)))
32+
require.Equal(t, cStats, *(*stats)(unsafe.Pointer(&runtimeStats)))
33+
}

0 commit comments

Comments
 (0)