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

ref(backtrace): add entries and extra logic for in-app detection #756

Merged
merged 4 commits into from
Mar 20, 2025

Conversation

lcian
Copy link
Member

@lcian lcian commented Mar 20, 2025

Description

Closes #717

@@ -13,11 +14,14 @@ const WELL_KNOWN_SYS_MODULES: &[&str] = &[
// these are not modules but things like __rust_maybe_catch_panic
"__rust_",
"___rust_",
"rust_begin_unwind",
Copy link
Member Author

Choose a reason for hiding this comment

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

This is always the top frame for panics and ends up in the high level issue description, I would rather have the first in-app frame there
Unless it's intended that we have rust_begin_unwind there?

Comment on lines +23 to +24
"futures_core::",
"futures_util::",
Copy link
Member Author

Choose a reason for hiding this comment

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

We should not add too specific stuff but this seems common enough to be here

@@ -111,7 +114,10 @@ pub fn function_starts_with(mut func_name: &str, mut pattern: &str) -> bool {
}
}
} else {
func_name = func_name.trim_start_matches('<').trim_start_matches("_<");
func_name = func_name
.trim_start_matches("<F as ")
Copy link
Member Author

@lcian lcian Mar 20, 2025

Choose a reason for hiding this comment

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

Specific case that can be commonly observed in raw frames related to futures_core, futures_util, axum, hyper, and others
It's specific for those cases as in general we will see the form <X as Y> for trait impls but I don't see any downside with adding this.

@lcian lcian requested a review from Swatinem March 20, 2025 14:11
Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

detecting this in the SDK requires these symbols to be present at runtime, which is often not the case when one is using a stripped release build.

Ideally the border frame should be added to our default stack trace enhancement rules that are part of serverside grouping, and are also applied after serverside symbolication.

@lcian
Copy link
Member Author

lcian commented Mar 20, 2025

detecting this in the SDK requires these symbols to be present at runtime, which is often not the case when one is using a stripped release build.

Ideally the border frame should be added to our default stack trace enhancement rules that are part of serverside grouping, and are also applied after serverside symbolication.

I'll do some more research and submit a PR to add more frames there.
There's at least one rule missing as currently when the SDK sends rust_begin_unwind as in app it's not overridden by the backend.

Just to document the change, the reason it's sent as in app right now is because in trim_stacktrace we trim on the first match.
If could happen that there's both std::sys::backtrace::__rust_end_short_backtrace and core::panicking::panic_fmt but we trim on the first of the two, leaving rust_begin_unwind as in_app between the two.
Screenshot 2025-03-20 at 16 45 44

@lcian lcian merged commit df0491c into master Mar 20, 2025
14 checks passed
@lcian lcian deleted the lcian/ref/backtrace-in-app branch March 20, 2025 15:50
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.

in_app_exclude does not work as intended.
2 participants