feat(code-mappings): Add support for stacktrace frames with backslashes for automatic code mappings #68845
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #68845 +/- ##
==========================================
- Coverage 79.72% 79.72% -0.01%
==========================================
Files 6422 6422
Lines 284675 284697 +22
Branches 49043 49045 +2
==========================================
+ Hits 226963 226978 +15
- Misses 57346 57353 +7
Partials 366 366
|
| @@ -61,15 +61,19 @@ class UnsupportedFrameFilename(Exception): | |||
| class FrameFilename: | |||
| def __init__(self, frame_file_path: str) -> None: | |||
| self.raw_path = frame_file_path | |||
| if frame_file_path[0] == "/": | |||
| is_windows_path = False | |||
| if "\\" in frame_file_path: | |||
There was a problem hiding this comment.
can this conflict with the escape character on Linux? e.g: \ as an escape for a space in the path? or is that just a shell thing?
There was a problem hiding this comment.
This is an interesting edge case! I am admittedly not very knowledgable on Linux or Shell, but a quick search tells me that this seems like more of a shell thing than a Linux thing. I'm less sure about the probability of a backslash escape character being used in a the file path of a stack trace, though the safety checks should catch these and ensure it doesn't result in any faulty code mappings. I'll periodically check the logs generated by those safety checks to see how often this edge case actually occurs and what the best way to resolve them is.
| stack_root = code_mapping.stack_root | ||
| if "\\" in code_mapping.stack_root: | ||
| stack_root = code_mapping.stack_root.replace("\\", "/") | ||
|
|
There was a problem hiding this comment.
This function (convert_stacktrace_frame_path_to_source_path) contains the logic that applies a code mapping to a given stack trace and returns the source path to the GH file.
This needed to be augmented to normalize for backslashes in the stack root, as described in the PR description
MichaelSun48
changed the title
MichaelSun48
added
the
Trigger: Revert
|
PR reverted: f186916 |
…ackslashes for automatic code mappings (#68845)" This reverts commit 4ed24e4. Co-authored-by: MichaelSun48 <[email protected]>
…s with backslashes for automatic code mappings (#70164) This PR un-reverts (re-reverts 🤔) [this PR](#68845), which adds automatic code mapping support for events with backslashes in the file path. This PR was reverted out of an abundance of caution when the `derive_code_mappings` task was killing memcache in s4s during the response to #inc-716. This occurred because: * The resolution to #inc-716 involved opening the floodgates to a massive amount of backlogged events that needed to be processed * `derive_code_mappings` pings the cache twice per event to ensure that we only process one event per project per hour, and with no duplicate issues per 24 hours. * A little bit out of my wheelhouse, but s4s's memcache infrastructure is significantly less robust than SaaS After having a conversation with @beezz, we determined that this would be highly unlikely to occur in SaaS (and there would be much bigger problems in that scenario), even given the higher traffic we'd expect to see in this task as a result of allowing more kinds of events to be supported for automatic code mappings.
This PR updates the existing code mapping logic to support stack traces whose filepaths contained backslashes instead of forward slashes. This effectively adds automatic code mapping support for issues derived from Windows machines (for the existing supported languages).
I've made the choice to save stack roots in the code mappings using whichever slash delimiter that the original stack path used. For example, if we have the following stack path and source:
"D:\Users\code\sentry\models\release.py""src/sentry/models/release.py"Then the roots generated will be:
"D:\Users\code\"(note that this source root still uses backslashes instead of forward slashes)"src/"This should be less confusing to the end user since we explicitly say we are going to replace the
stack_rootin thestack_pathwith thesource_rootto get thesource_path. It might be confusing if thestack_root's slash-delimiter was not the same as that in thestack_path.This also means that the previous logic to apply code mappings to a given frame to generate a stacktrace link needed to be updated so that the source path generated would only include slashes (since this path is used in a github URL, which only uses forward slashes).