fix(stacktrace-link): Only code mappings that match should affect the API response #41562

armenzg · 2022-11-18T16:03:11Z

Changes:

Fix that the last code mapping would set the error information even if it didn't match
Make the code readable
Improve debuggability by returning all code mappings that match and associated results
Add organization.slug and project.slug tags

armenzg · 2022-11-18T16:05:26Z

src/sentry/api/endpoints/project_stacktrace_link.py

@@ -15,8 +15,9 @@


 def get_link(
-    config: RepositoryProjectPathConfig, filepath: str, default: str, version: Optional[str] = None
-) -> Tuple[Optional[str], Optional[str], Optional[str]]:
+    config: RepositoryProjectPathConfig, filepath: str, version: Optional[str] = None


Using Any instead of dict[str, str] because some weird issue I was not able to figure out.
I can try again in the future.

armenzg · 2022-11-18T16:07:14Z

src/sentry/api/endpoints/project_stacktrace_link.py

+
+# This is to support mobile languages with non-fully-qualified file pathing.
+# We attempt to 'munge' the proper source-relative filepath based on the stackframe data.
+def generate_mobile_frame(parameters: Any) -> Any:


I just moved the code from here into a function:

sentry/src/sentry/api/endpoints/project_stacktrace_link.py

Lines 67 to 76 in b0b214f

abs_path = request.GET.get("absPath")

module = request.GET.get("module")

package = request.GET.get("package")

frame = {}

if abs_path:

frame["abs_path"] = abs_path

if module:

frame["module"] = module

if package:

frame["package"] = package

armenzg · 2022-11-18T16:07:59Z

src/sentry/api/endpoints/project_stacktrace_link.py

@@ -56,24 +109,21 @@ class ProjectStacktraceLinkEndpoint(ProjectEndpoint):
    """

    def get(self, request: Request, project) -> Response:
+        # import pprint
+
+        # pprint.pp(request)


I will remove this in the following push.

armenzg · 2022-11-18T16:08:22Z

src/sentry/api/endpoints/project_stacktrace_link.py

@@ -85,69 +135,63 @@ def get(self, request: Request, project) -> Response:
            if i.has_feature(IntegrationFeatures.STACKTRACE_LINK)
        ]

-        last_error = None


Not needed anymore.

armenzg · 2022-11-18T16:09:53Z

src/sentry/api/endpoints/project_stacktrace_link.py

-
-                            link, attempted_url, error = get_link(
-                                config, munged_filename, config.default_branch, commit_id
-                            )


This whole block is now done above like this:

# For mobile we try a second time by munging the file path if not result["sourceUrl"] and mobile_frame: result = try_path_munging(filepath, mobile_frame, ctx)

armenzg · 2022-11-18T16:10:42Z

src/sentry/api/endpoints/project_stacktrace_link.py

-                    last_error = get_link_error
-                    result["attemptedUrl"] = attempted_url
+                if not result["sourceUrl"]:
+                    matched_code_mappings.append(current_config)


This will create a list of all code mappings that were attempted and its result.

armenzg · 2022-11-18T17:57:36Z

src/sentry/api/endpoints/project_stacktrace_link.py

@@ -15,27 +15,77 @@


 def get_link(
-    config: RepositoryProjectPathConfig, filepath: str, default: str, version: Optional[str] = None


config contains the default branch. I'm reducing the signature.

armenzg · 2022-11-18T17:59:34Z

src/sentry/api/endpoints/project_stacktrace_link.py

+    return frame
+
+
+def try_path_munging(


This function comes from the main loop.

sentry/src/sentry/api/endpoints/project_stacktrace_link.py

Lines 114 to 130 in b0b214f

frame["filename"] = filepath

munged_frames = munged_filename_and_frames(

platform, [frame], "munged_filename", sdk_name=sdk_name

)

if munged_frames:

munged_frame: Mapping[str, Any] = munged_frames[1][0]

munged_filename = str(munged_frame.get("munged_filename"))

if munged_filename:

if not filepath.startswith(

config.stack_root

) and not munged_filename.startswith(config.stack_root):

last_error = "stack_root_mismatch"

continue

link, attempted_url, error = get_link(

config, munged_filename, config.default_branch, commit_id

)

armenzg · 2022-11-18T18:00:53Z

src/sentry/api/endpoints/project_stacktrace_link.py

+            "platform": request.GET.get("platform"),
+            "sdk_name": request.GET.get("sdkName"),
+        }
+        mobile_frame = generate_mobile_frame(request.GET)


Using mobile_frame later in the code to decide if this is a mobile platform does not make sense. Either way, here we are.

armenzg · 2022-11-18T18:01:24Z

src/sentry/api/endpoints/project_stacktrace_link.py

        with configure_scope() as scope:
+            scope.set_tag("project.slug", project.slug)
+            scope.set_tag("organization.slug", project.organization.slug)


These are two new tags.

armenzg · 2022-11-18T18:04:43Z

tests/sentry/api/endpoints/test_project_stacktrace_link.py

@@ -116,10 +116,8 @@ def test_file_not_found_error(self):
        )
        assert response.data["config"] == self.expected_configurations(self.code_mapping1)
        assert not response.data["sourceUrl"]
-        # XXX: This depends on what was the last attempted code mapping
-        assert response.data["error"] == "stack_root_mismatch"
+        assert response.data["error"] == "file_not_found"


In #41409 I added more than one code mapping and it showed that the error value would depend on the last attempted code mapping rather than the last one that matched.

This PR fixes the issue since the first code mapping matches the stack trace root and the source file is not found (as the original test was):

sentry/tests/sentry/api/endpoints/test_project_stacktrace_link.py

Line 58 in 78a95d5

stack_root="usr/src/getsentry/",

ref(stacktrace-link): Clean up the code and improvements

8de390b

armenzg self-assigned this Nov 18, 2022

armenzg added this to the Derived Code Mappings (EA release - Python only) milestone Nov 18, 2022

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 18, 2022

armenzg commented Nov 18, 2022

View reviewed changes

armenzg added 2 commits November 18, 2022 12:47

More fixes

15bf240

Fix last tests

363a73b

vercel bot deployed to Preview – sentry November 18, 2022 17:59 View deployment

vercel bot deployed to Preview – storybook November 18, 2022 18:00 View deployment

armenzg commented Nov 18, 2022

View reviewed changes

armenzg changed the title ~~ref(stacktrace-link): Clean up the code and improvements~~ fix(stacktrace-link): Only code mappings that match should affect the API response Nov 18, 2022

armenzg marked this pull request as ready for review November 18, 2022 18:24

armenzg requested a review from a team as a code owner November 18, 2022 18:24

armenzg requested review from a team November 18, 2022 18:24

snigdhas approved these changes Nov 18, 2022

View reviewed changes

armenzg merged commit cb37060 into master Nov 21, 2022

armenzg deleted the armenzg/derive-code-mappings/track-invalid-code-mappings branch November 21, 2022 13:24

github-actions bot locked and limited conversation to collaborators Dec 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(stacktrace-link): Only code mappings that match should affect the API response #41562

fix(stacktrace-link): Only code mappings that match should affect the API response #41562

armenzg commented Nov 18, 2022 •

edited

Loading

armenzg Nov 18, 2022 •

edited

Loading

armenzg Nov 18, 2022

armenzg Nov 18, 2022

armenzg Nov 18, 2022

armenzg Nov 18, 2022

armenzg Nov 18, 2022

armenzg Nov 18, 2022

armenzg Nov 18, 2022

armenzg Nov 18, 2022

armenzg Nov 18, 2022

armenzg Nov 18, 2022

	abs_path = request.GET.get("absPath")
	module = request.GET.get("module")
	package = request.GET.get("package")
	frame = {}
	if abs_path:
	frame["abs_path"] = abs_path
	if module:
	frame["module"] = module
	if package:
	frame["package"] = package

		@@ -15,27 +15,77 @@


		def get_link(
		config: RepositoryProjectPathConfig, filepath: str, default: str, version: Optional[str] = None

	frame["filename"] = filepath
	munged_frames = munged_filename_and_frames(
	platform, [frame], "munged_filename", sdk_name=sdk_name
	)
	if munged_frames:
	munged_frame: Mapping[str, Any] = munged_frames[1][0]
	munged_filename = str(munged_frame.get("munged_filename"))
	if munged_filename:
	if not filepath.startswith(
	config.stack_root
	) and not munged_filename.startswith(config.stack_root):
	last_error = "stack_root_mismatch"
	continue

	link, attempted_url, error = get_link(
	config, munged_filename, config.default_branch, commit_id
	)

fix(stacktrace-link): Only code mappings that match should affect the API response #41562

fix(stacktrace-link): Only code mappings that match should affect the API response #41562

Conversation

armenzg commented Nov 18, 2022 • edited Loading

armenzg Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armenzg commented Nov 18, 2022 •

edited

Loading

armenzg Nov 18, 2022 •

edited

Loading