feat: Add reason for in_app to frames #3672
Conversation
Sentry enhances the in_app decision from the SDK, sometimes overriding it. In cases the decision to set in_app to True comes from the user themselves (by setting in_app_include), we don't want Sentry to override the decision. Currently there's no way for Sentry to know what led to the decision in the SDK. Changing that here.
There was a problem hiding this comment.
This will definitely make things easier for us on the server end - thank you!
I think it'd actually be helpful to make it a bit more general, though. As someone reading this data, my main question is, "Can I override the value here?," and I can think of three scenarios:
- Frame gets marked in- or out-of-app automatically using rules baked into the SDK.
- Frame gets marked in- or out-of-app according to the include/exclude options set by the user in SDK config.
- Frame gets marked in- or out-of-app manually by the user in
before_send.
In the first scenario, the server can overwrite the value, but in the second two, it shouldn't. So what would be really helpful would be to know 1) why was it marked this way, and 2) how was it marked (separate from the actual value, in case it gets changed in before_send).
One option, which would give nice parity with what we do on the server side, would be to store the "rule" which was matched, so it would end up being something like in_app_include: <matching pattern>/in_app_exclude: <matching pattern> and auto in_app_include: <matching pattern>/auto in_app_exclude: <matching pattern>.
If we had that, a) we could show it along with the matched server rules in grouping info, and b) the server would know it could overwrite any value with auto (except if the auto conclusion is different from the in_app value, because that would indicate it'd been overridden manually by the user).
Thoughts?
| frame["in_app"] = True | ||
| frame["in_app_include_used"] = include_pattern_matched |
There was a problem hiding this comment.
Why store this info for an include but not for an exclude?
| @@ -1118,18 +1120,18 @@ def event_from_exception( | |||
|
|
|||
|
|
|||
| def _module_in_list(name, items): | |||
There was a problem hiding this comment.
Should we change the name of this function to better match it's new job - something like _find_module_match_in_list or some such?
|
Hey @lobsterkatie, thanks for the feedback on this but we will not go forward with this PR (see #3671 (comment)). Instead, we'll try to fix the more fundamental problem of why Sentry can't rely on the decision from the SDK in the first place, which will hopefully make this change redundant. 🤞🏻 |
|
Superseded by getsentry/sentry#83603 |
Sentry enhances the
in_appdecision from the SDK, sometimes overriding it.In case the decision to set
in_apptoTruecomes from the user themselves (by settingin_app_include), we don't want Sentry to override it.Currently there's no way for Sentry to know what led to the decision in the SDK. Changing that here.
Draft: To be clarified whether
in_app_include_usedshould contain the actual prefix from the user'sin_app_includeor a general category likein_project_root,in_app_includeoutlining the general reason for marking it as in app.Closes #3671