Skip to content

feat: Add integration for parsing GNU-style backtrace dumps #288

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

Merged
merged 4 commits into from
Mar 18, 2019

Conversation

untitaker
Copy link
Member

@untitaker untitaker commented Mar 10, 2019

Value proposition: ClickHouse sends us error messages with stacktrace information. Ideally we would parse this stacktrace information into actual stacktraces. Here is an integration for exactly that. The testcase is ripped straight from https://sentry.io/organizations/sentry/issues/856036064

ClickHouse calls backtrace_symbols. It already returns a formatted string/list of strings. ClickHouse then parses this string and replaces symbols with demangled versions. I didn't bother with parsing addresses and offsets as I don't see much value in that information (not sure if it's used in grouping) and some of those return addresses (brackets at the end) seem wrong:

23. clickhouse-server() [0xa3c68cf]
24. /lib/x86_64-linux-gnu/libpthread.so.0(+0x8184) [0x7fdb108c5184]
25. /lib/x86_64-linux-gnu/libc.so.6(clone+0x6d) [0x7fdb0fee003d]

I'm open to alternative naming suggestions. As far as I can tell this function has its origins in glibc. LibExecInfoBacktraceIntegration seemed bulky. Perhaps GlibcBacktraceIntegration to avoid conflict with GDB's output format, which seems entirely different? I'm generally unsure of backtrace_symbols's origin. It seems to have started as a glibc thing but there are ports for BSD and Darwin. It isn't POSIX though.

When discussing the idea of this with @mitsuhiko there was the idea to have a single NativeStacktraceIntegration where we would add new kinds of stacktrace formats over time. However, in order to not break grouping between minor versions, we can't actually add new variants to that "super-integration" without having the user explicitly opt into each of them (just the same reason why we can't make this a default integration). Therefore I see no point in having one integration for multiple formats, and the naming problem persists.

cc @alex-hofsteede

@untitaker untitaker requested review from mitsuhiko and jan-auer March 10, 2019 20:55
@untitaker
Copy link
Member Author

I wonder if we can share this code across SDKs somehow. There's nothing in this integration that couldn't apply to nodejs or C#. I considered writing it in Haxe.

@untitaker untitaker requested review from bruno-garcia and removed request for jan-auer March 17, 2019 13:44
@untitaker
Copy link
Member Author

@bruno-garcia adding you because of what we discussed the last retros about SDK work.

@bruno-garcia
Copy link
Member

@untitaker do you have a screenshot of what it looks like in Sentry the mix callstack? If you do, could you paste in the PR, please?

@untitaker
Copy link
Member Author

untitaker commented Mar 18, 2019

@untitaker
Copy link
Member Author

untitaker commented Mar 18, 2019

@mitsuhiko reminded me to set platform to native, will fix

@bruno-garcia
Copy link
Member

@untitaker we ahve new stacktrace rendering now? :)

@untitaker
Copy link
Member Author

@bruno-garcia I updated the screenshot

@untitaker
Copy link
Member Author

One shitty aspect of this right now is that every frame is annotated as inline frame because there are no instruction addrs

@jan-auer
Copy link
Member

@untitaker We can change that quite easily. Should actually.

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

OK with changing the instruction addr this (wrt inline frame) on a separate PR.

@untitaker untitaker merged commit 1d78eb0 into master Mar 18, 2019
@untitaker untitaker deleted the feat/gnu-backtrace-integration branch March 18, 2019 15:25
sentrivana pushed a commit that referenced this pull request Jul 22, 2025
This integration was added back in 2019
#288 but I think since
then the stack trace from clickhouse driver has changed, anyway I tried
to update it so that it parses the stack trace again
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.

3 participants