Skip to content

feat(code-mappings): Add new function for applying code mappings to stack frames (try 2)#63128

Merged
malwilley merged 2 commits intomasterfrom
malwilley/feat/code-mappings-common-func
Jan 12, 2024
Merged

feat(code-mappings): Add new function for applying code mappings to stack frames (try 2)#63128
malwilley merged 2 commits intomasterfrom
malwilley/feat/code-mappings-common-func

Conversation

@malwilley
Copy link
Member

@malwilley malwilley commented Jan 12, 2024

Re-submitting #62971 because it broke CI and had to be reverted. Pasting PR description below:

There are a couple problems with the current code mapping flow that are addressed in this PR:

  1. Code mapping logic is duplicated (and slightly different) in stacktrace linking and suspect commits.

To ensure a consistent experience, stacktrace linking and suspect commits should apply code mappings in a similar way. By introducing a new function convert_stacktrace_frame_path_to_source_path(), we can use it in both locations to guarantee that the implementations do not diverge. (Note that this function is tested but not yet used - will update stacktrace linking and suspect commits in a separate PR)

  1. Code mappings only apply to filename, not abs_path

Certain platforms have trouble creating valid code mappings because filename only contains the file name and the folder structure is in abs_path (see #43516 (comment)). By adding it as a fallback check in convert_stacktrace_frame_path_to_source_path() we can support these platforms.

Related to the above concern with abs_path, I also modified get_sorted_code_mapping_configs() to check for absolute paths in the stack_root while sorting code mappings. Without this check, the sorting does not work as expected.

@malwilley malwilley requested a review from a team January 12, 2024 18:19
@malwilley malwilley requested a review from a team as a code owner January 12, 2024 18:19
@malwilley malwilley changed the title feat(code-mappings): Add new function for applying code mappings to stack frames (try #2) feat(code-mappings): Add new function for applying code mappings to stack frames (try 2) Jan 12, 2024
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jan 12, 2024
Copy link
Member

@wedamija wedamija left a comment

Choose a reason for hiding this comment

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

Approving since this is the same as #62971

@codecov
Copy link

codecov bot commented Jan 12, 2024

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (c0e45d0) 81.35% compared to head (0b0050a) 81.35%.
Report is 3 commits behind head on master.

❗ Current head 0b0050a differs from pull request most recent head 6e5b301. Consider uploading reports for the commit 6e5b301 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #63128   +/-   ##
=======================================
  Coverage   81.35%   81.35%           
=======================================
  Files        5205     5205           
  Lines      230706   230732   +26     
  Branches    39862    39868    +6     
=======================================
+ Hits       187682   187706   +24     
- Misses      37267    37268    +1     
- Partials     5757     5758    +1     
Files Coverage Δ
src/sentry/integrations/utils/code_mapping.py 92.79% <75.00%> (-0.63%) ⬇️
src/sentry/utils/event_frames.py 91.37% <91.66%> (+1.58%) ⬆️

... and 4 files with indirect coverage changes

@malwilley malwilley enabled auto-merge (squash) January 12, 2024 19:17
@malwilley malwilley merged commit a502309 into master Jan 12, 2024
@malwilley malwilley deleted the malwilley/feat/code-mappings-common-func branch January 12, 2024 19:36
Copy link
Member

@armenzg armenzg left a comment

Choose a reason for hiding this comment

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

This is great to see!

trillville pushed a commit that referenced this pull request Jan 19, 2024
…tack frames (try 2) (#63128)

There are a couple problems with the current code mapping flow that are
addressed in this commit:

1. Code mapping logic is duplicated (and slightly different) in
stacktrace linking and suspect commits.

To ensure a consistent experience, stacktrace linking and suspect
commits should apply code mappings in a similar way. By introducing a
new function convert_stacktrace_frame_path_to_source_path(), we can use
it in both locations to guarantee that the implementations do not
diverge. (Note that this function is tested but not yet used - will
update stacktrace linking and suspect commits in a separate PR)

2. Code mappings only apply to filename, not abs_path

Certain platforms have trouble creating valid code mappings because
filename only contains the file name and the folder structure is in
abs_path (see
#43516 (comment)).
By adding it as a fallback check in
convert_stacktrace_frame_path_to_source_path() we can support these
platforms.

Related to the above concern with abs_path, I also modified
get_sorted_code_mapping_configs() to check for absolute paths in the
stack_root while sorting code mappings. Without this check, the sorting
does not work as expected.
@github-actions github-actions bot locked and limited conversation to collaborators Jan 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants