Skip to content
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.

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

symbolizer: shell out to addr2line #5299

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

danipozo
Copy link

Use addr2line instead of custom addr2line implementation which doesn't symbolize addresses properly for some binaries (see #5291). Even with a fix for go-delve/delve#3861, which is the root cause of the failure in #5291, the current implementation can't symbolize ~50 % of the addresses in the stacks of the ClickHouse binary in my tests. Using gimli-rs/addr2line as the addr2line implementation works fast and well:
image

Please take this PR as a rough proposal, there are of course details to be fleshed out:

  • Probably you want to keep the current implementation under a setting
  • If not, probably some code would need to be deleted
  • This introduces another dependency, docs and Docker images at least would have to be updated
  • Also there are likely code issues, I'm far from a Go expert

@danipozo danipozo requested a review from a team as a code owner November 19, 2024 18:35
@brancz
Copy link
Member

brancz commented Dec 6, 2024

I'm open to the idea of this though I would much rather actually fix the symbolizer, but I'd be ok with this being an escape hatch. It's definitely possible to write a correct symbolizer even with the bug in the delve code (we did in Polar Signals Colud but what we did doesn't work in a single-process context that we're constrained to in Parca).

This being an escape hatch, I would prefer if we only implement support in Parca for this, but do not add llvm-addr2line or something like that to the container image, we would expect someone to add that themselves if they want to use this escape hatch until the symbolizer itself is fixed.

@danipozo
Copy link
Author

danipozo commented Dec 9, 2024

Nice to read, I'll add a setting for this then and maybe some docs on how to use it if you face broken symbolization for your binary

Adds symbolizer flag to specify addr2line binary to shell out to. Create
a new liner type that uses said binary.
@danipozo
Copy link
Author

I've added a setting to enable this functionality by specifying a path to the addr2line binary, and a new liner that uses this binary. This liner can also be wrapped by the cachedLiner and therefore use the cache.

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