[crashtracker] Take relative address and build id for remote symbolication #473
Conversation
danielsn
force-pushed
the
dsn/crashtracker-relative-addresses
branch
from
1bd5341
to
8367483
Compare
| @@ -325,12 +327,15 @@ impl<'a> TryFrom<&StackFrame<'a>> for datadog_crashtracker::StackFrame { | |||
| } | |||
| Some(vec) | |||
| }; | |||
| let relative_address = to_hex(value.relative_address); | |||
There was a problem hiding this comment.
Is this valid to assume a relative offset of 0 is None? Or do we need to be more clever here?
There was a problem hiding this comment.
the term relative is ambiguous, as it can have different meanings.
we use elf, blazesym uses normalized. I can imagine you are aiming for cross compatibility ?
0 for an elf address is not possible (there are headers)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #473 +/- ##
==========================================
- Coverage 69.55% 69.37% -0.19%
==========================================
Files 200 200
Lines 26656 26726 +70
==========================================
Hits 18541 18541
- Misses 8115 8185 +70
|
There was a problem hiding this comment.
We should need "just" these 2 pieces of information, but I suspect most of the complexity is in deriving the elf address, which could be helpful to exist in libdatadog. Nicolas and Erwan would be the experts here and their code in ddprof probably would be useful to reference
| if let Some(ip) = &self.ip { | ||
| let ip = ip.trim_start_matches("0x"); | ||
| let ip = u64::from_str_radix(ip, 16)?; | ||
| let normed = normalizer.normalize_user_addrs(pid, &[ip])?; |
There was a problem hiding this comment.
Makes sense to me based on my reading of the blazesym docs - https://docs.rs/blazesym/latest/i686-unknown-linux-gnu/blazesym/normalize/struct.Normalizer.html
| UserMeta::Apk(a) => Self::Apk(a.path.clone()), | ||
| UserMeta::Elf(e) => Self::Elf { | ||
| path: e.path.clone(), | ||
| build_id: e.build_id.clone(), |
There was a problem hiding this comment.
Does this handle go build id, which can be in a different part of the metadata than the GNU build id? Or is this irrelevant because we don't currently use libdatadog in dd-trace-go?
There was a problem hiding this comment.
I'd guess the second, but Felix/Nick may have thoughts. If they want it, I'll add it in a followup PR
What does this PR do?
Adds two new fields optional to each stack trace frame:
relative_addressandbuild_idMotivation
This is the information required by a backend symbolizer
Additional Notes
Getting relative addresses from absolute addresses is the responsibility of the client. In the future, we may add functionality to libdatadog to support this.
How to test the change?
Describe here in detail how the change can be validated.