-
Notifications
You must be signed in to change notification settings - Fork 571
[do not merge] Only mark frame as in_app if explicitly set by user
#3984
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #3984 +/- ##
==========================================
- Coverage 80.20% 80.18% -0.03%
==========================================
Files 139 139
Lines 15394 15385 -9
Branches 2596 2593 -3
==========================================
- Hits 12347 12336 -11
- Misses 2202 2203 +1
- Partials 845 846 +1
|
in_app if explicitly set by userin_app if explicitly set by user
lobsterkatie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. One question below, and one which I couldn't put in the right spot because it's not near enough to a change, which is:
# if frame has already been marked as in_app, skip it
current_in_app = frame.get("in_app")
if current_in_app is not None:
continueHow is it that a frame could come in with an in-app value already set?
| ["main"], | ||
| None, | ||
| { | ||
| "abs_path": "C:\\Users\\winuser\\AppData\\Roaming\\Python\\Python35\\site-packages\\fastapi\\routing.py", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually partially answers a question I had, which is how windows paths get reported. Is it safe to assume that if the paths in the frames are reported with the C: and with double backslashes, the project root will be, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd guess so -- if the two didn't match then the is-in-project-root check would never be correct.
I am not sure, which is why I opted for not changing it. It shouldn't hurt in any case. |
❗ This is ready for review, but not ready to be merged. Releasing this needs to be coordinated with (1) the SDK sending
project_root(#3941) and (2) the logic actually being moved to the server.Only take
in_app_includeandin_app_excludeinto account when marking a framein_app, nothing else. The existing logic will be moving to the server. We will not merge this before that happens.See getsentry/sentry#83603 for the big picture
Closes #3945