Skip to content

drop host.TraceHash #341

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
florianl wants to merge 2 commits into from
Closed

drop host.TraceHash #341

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cab1723
host: drop 64 bit TraceHash
florianl Feb 4, 2025
0e3ee46
tracehandler: remodel traceCache
florianl Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions host/host.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,6 @@ import (
"go.opentelemetry.io/ebpf-profiler/times"
)

// TraceHash is used for unique identifiers for traces, and is required to be 64-bits
// due to the constraints imposed by the eBPF maps, unlike the larger TraceHash used
// outside the host agent.
type TraceHash uint64

// FileID is used for unique identifiers for files, and is required to be 64-bits
// due to the constraints imposed by the eBPF maps, unlike the larger FileID used
// outside the host agent.
Expand Down Expand Up @@ -51,7 +46,7 @@ type Trace struct {
ProcessName string
ExecutablePath string
Frames []Frame
Hash TraceHash
Hash libpf.TraceHash
KTime times.KTime
PID libpf.PID
TID libpf.PID
Expand Down
2 changes: 1 addition & 1 deletion processmanager/manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ func TestInterpreterConvertTrace(t *testing.T) {
// trace for tests below.
func getExpectedTrace(origTrace *host.Trace, linenos []libpf.AddressOrLineno) *libpf.Trace {
newTrace := &libpf.Trace{
Hash: libpf.NewTraceHash(uint64(origTrace.Hash), uint64(origTrace.Hash)),
Hash: origTrace.Hash,
}

for _, frame := range origTrace.Frames {
Expand Down
21 changes: 10 additions & 11 deletions tracehandler/tracehandler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,9 @@ type traceHandler struct {

traceProcessor TraceProcessor

// bpfTraceCache stores mappings from BPF to user-mode hashes. This allows
// avoiding the overhead of re-doing user-mode symbolization of traces that
// we have recently seen already.
bpfTraceCache *lru.LRU[host.TraceHash, libpf.TraceHash]
// traceCache avoids the overhead of re-doing user-mode symbolization of traces
// that we have recently seen already.
traceCache *lru.LRU[libpf.TraceHash, libpf.Void]

// umTraceCache is a LRU set that suppresses unnecessary resends of traces
// that we have recently reported to the collector already.
Expand All @@ -87,8 +86,8 @@ type traceHandler struct {
// newTraceHandler creates a new traceHandler
func newTraceHandler(rep reporter.TraceReporter, traceProcessor TraceProcessor,
intervals Times, cacheSize uint32) (*traceHandler, error) {
bpfTraceCache, err := lru.New[host.TraceHash, libpf.TraceHash](
cacheSize, func(k host.TraceHash) uint32 { return uint32(k) })
traceCache, err := lru.New[libpf.TraceHash, libpf.Void](
cacheSize, func(k libpf.TraceHash) uint32 { return k.Hash32() })
if err != nil {
return nil, err
}
Expand All @@ -107,7 +106,7 @@ func newTraceHandler(rep reporter.TraceReporter, traceProcessor TraceProcessor,

t := &traceHandler{
traceProcessor: traceProcessor,
bpfTraceCache: bpfTraceCache,
traceCache: traceCache,
umTraceCache: umTraceCache,
reporter: rep,
times: intervals,
Expand All @@ -133,11 +132,11 @@ func (m *traceHandler) HandleTrace(bpfTrace *host.Trace) {

if !m.reporter.SupportsReportTraceEvent() {
// Fast path: if the trace is already known remotely, we just send a counter update.
postConvHash, traceKnown := m.bpfTraceCache.Get(bpfTrace.Hash)
Copy link
Copy Markdown
Member

@christos68k christos68k Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this change our 128bit hashes we report to the backend completely?

Essentially, the hashing logic in ConvertTrace is no longer used, in favor of

tmpHash := xxh3.Hash128(raw)
trace.Hash = libpf.NewTraceHash(tmpHash.Hi, tmpHash.Lo)

which has different characteristics and cardinality as it's our 64bit hash extended. This is probably not what we want for the 128bit hash.

Copy link
Copy Markdown
Member Author

@florianl florianl Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it does not affect the hash that is reported to the backend. If it would change the reported hash, this proposed change would be a breaking change.

The hash, that is used to report back to the backend is untouched and happens in ConvertTrace() after the symbolization:

opentelemetry-ebpf-profiler/processmanager/manager.go

Line 292 in 48471c0

newTrace.Hash = traceutil.HashTrace(newTrace)

Copy link
Copy Markdown
Member

@christos68k christos68k Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we're calling in line 139:

m.reporter.ReportCountForTrace(bpfTrace.Hash, 1, meta)

bpfTrace.Hash is the extended 64bit hash, not the post-conversion hash.

Copy link
Copy Markdown
Member Author

@florianl florianl Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch 👍

The hash returned by ConvertTrace() is stored for translation in bpfTraceCache and therefore this should not change. I will close the PR.

_, traceKnown := m.traceCache.Get(bpfTrace.Hash)
if traceKnown {
m.bpfTraceCacheHit++
meta.APMServiceName = m.traceProcessor.MaybeNotifyAPMAgent(bpfTrace, postConvHash, 1)
m.reporter.ReportCountForTrace(postConvHash, 1, meta)
meta.APMServiceName = m.traceProcessor.MaybeNotifyAPMAgent(bpfTrace, bpfTrace.Hash, 1)
m.reporter.ReportCountForTrace(bpfTrace.Hash, 1, meta)
return
}
m.bpfTraceCacheMiss++
Expand All @@ -146,7 +145,7 @@ func (m *traceHandler) HandleTrace(bpfTrace *host.Trace) {
// Slow path: convert trace.
umTrace := m.traceProcessor.ConvertTrace(bpfTrace)
log.Debugf("Trace hash remap 0x%x -> 0x%x", bpfTrace.Hash, umTrace.Hash)
m.bpfTraceCache.Add(bpfTrace.Hash, umTrace.Hash)
m.traceCache.Add(bpfTrace.Hash, libpf.Void{})

meta.APMServiceName = m.traceProcessor.MaybeNotifyAPMAgent(bpfTrace, umTrace.Hash, 1)
if m.reporter.SupportsReportTraceEvent() {
Expand Down
18 changes: 9 additions & 9 deletions tracehandler/tracehandler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ var _ tracehandler.TraceProcessor = (*fakeTraceProcessor)(nil)

func (f *fakeTraceProcessor) ConvertTrace(trace *host.Trace) *libpf.Trace {
var newTrace libpf.Trace
newTrace.Hash = libpf.NewTraceHash(uint64(trace.Hash), uint64(trace.Hash))
newTrace.Hash = trace.Hash
return &newTrace
}

Expand Down Expand Up @@ -101,23 +101,23 @@ func TestTraceHandler(t *testing.T) {

// simulates a single trace being received.
"single trace": {input: []arguments{
{trace: &host.Trace{Hash: host.TraceHash(0x1234)}},
{trace: &host.Trace{Hash: libpf.NewTraceHash(0x12, 0x34)}},
},
expectedTraces: []reportedTrace{{traceHash: libpf.NewTraceHash(0x1234, 0x1234)}},
expectedTraces: []reportedTrace{{traceHash: libpf.NewTraceHash(0x12, 0x34)}},
expectedCounts: []reportedCount{
{traceHash: libpf.NewTraceHash(0x1234, 0x1234), count: 1},
{traceHash: libpf.NewTraceHash(0x12, 0x34), count: 1},
},
},

// double trace simulates a case where the same trace is encountered in quick succession.
"double trace": {input: []arguments{
{trace: &host.Trace{Hash: host.TraceHash(4)}},
{trace: &host.Trace{Hash: host.TraceHash(4)}},
{trace: &host.Trace{Hash: libpf.NewTraceHash(0x56, 0x78)}},
{trace: &host.Trace{Hash: libpf.NewTraceHash(0x56, 0x78)}},
},
expectedTraces: []reportedTrace{{traceHash: libpf.NewTraceHash(4, 4)}},
expectedTraces: []reportedTrace{{traceHash: libpf.NewTraceHash(0x56, 0x78)}},
expectedCounts: []reportedCount{
{traceHash: libpf.NewTraceHash(4, 4), count: 1},
{traceHash: libpf.NewTraceHash(4, 4), count: 1},
{traceHash: libpf.NewTraceHash(0x56, 0x78), count: 1},
{traceHash: libpf.NewTraceHash(0x56, 0x78), count: 1},
},
},
}
Expand Down
3 changes: 2 additions & 1 deletion tracer/tracer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1007,7 +1007,8 @@ func (t *Tracer) loadBpfTrace(raw []byte, cpu int) *host.Trace {
ptr.ktime = 0
ptr.origin = 0
ptr.offtime = 0
trace.Hash = host.TraceHash(xxh3.Hash128(raw).Lo)
tmpHash := xxh3.Hash128(raw)
trace.Hash = libpf.NewTraceHash(tmpHash.Hi, tmpHash.Lo)

userFrameOffs := 0
if ptr.kernel_stack_id >= 0 {
Expand Down