Skip to content

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

Merged
untitaker merged 4 commits into
masterfrom
feat/gnu-backtrace-integration
Mar 18, 2019
Merged

feat: Add integration for parsing GNU-style backtrace dumps#288
untitaker merged 4 commits into
masterfrom
feat/gnu-backtrace-integration

Conversation

@untitaker

@untitaker untitaker commented Mar 10, 2019

Copy link
Copy Markdown
Member

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 jan-auer and mitsuhiko March 10, 2019 20:55
@untitaker

Copy link
Copy Markdown
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
Copy Markdown
Member Author

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

@bruno-garcia

Copy link
Copy Markdown
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

untitaker commented Mar 18, 2019

Copy link
Copy Markdown
Member Author

@untitaker

untitaker commented Mar 18, 2019

Copy link
Copy Markdown
Member Author

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

@bruno-garcia

Copy link
Copy Markdown
Member

@untitaker we ahve new stacktrace rendering now? :)

@untitaker

Copy link
Copy Markdown
Member Author

@bruno-garcia I updated the screenshot

@untitaker

Copy link
Copy Markdown
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
Copy Markdown
Member

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

@bruno-garcia bruno-garcia left a comment

Copy link
Copy Markdown
Member

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