Skip to content

drop host.TraceHash#341

Closed
florianl wants to merge 2 commits intomainfrom
drop-host.TraceHash
Closed

drop host.TraceHash#341
florianl wants to merge 2 commits intomainfrom
drop-host.TraceHash

Conversation

@florianl
Copy link
Copy Markdown
Member

@florianl florianl commented Feb 4, 2025

This is a follow up to #340.

When working on #340 I realized we still differentiate between hashes on a host (64 bit) and global hashes (128 bit). With the rework on how traces and their counts are reported to user space, this differentiation is no longer needed.
Therefore we can use just one kind of hash, reduce the memory footprint of a map that converts these hashes and avoid additional computing for hashes.

A 64 bit TraceHash was used to differentiate between hashes on a host vs hashes on a global context. The later one are 128 bit hashes.

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
bpfTraceCache used to be essential for the eBPF to user space communication at the time when only hashes and their respective count were reported. As this workflow got refactores, this is no longer required.

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
@florianl florianl requested review from a team as code owners February 4, 2025 10:59

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

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

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:

newTrace.Hash = traceutil.HashTrace(newTrace)

Copy link
Copy Markdown
Member

@christos68k christos68k Feb 4, 2025

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

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.

@florianl florianl closed this Feb 4, 2025
@florianl florianl deleted the drop-host.TraceHash branch February 4, 2025 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants