-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(auto_source): Derive a code mapping per module #101288
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,9 +2,10 @@ | |
|
|
||
| import re | ||
| from abc import ABC, abstractmethod | ||
| from collections.abc import Mapping | ||
| from collections.abc import Mapping, Sequence | ||
| from typing import Any | ||
|
|
||
| from sentry import options | ||
| from sentry.integrations.source_code_management.repo_trees import get_extension | ||
|
|
||
| from .constants import SECOND_LEVEL_TLDS, STACK_ROOT_MAX_LEVEL | ||
|
|
@@ -42,7 +43,7 @@ def __init__(self, frame: Mapping[str, Any]) -> None: | |
| self.process_frame(frame) | ||
|
|
||
| def __repr__(self) -> str: | ||
| return f"FrameInfo: {self.raw_path}" | ||
| return f"FrameInfo: {self.raw_path} stack_root: {self.stack_root}" | ||
|
|
||
| def __eq__(self, other: object) -> bool: | ||
| if not isinstance(other, FrameInfo): | ||
|
|
@@ -148,6 +149,14 @@ def get_path_from_module(module: str, abs_path: str) -> tuple[str, str]: | |
| # Gets rid of the class name | ||
| parts = module.rsplit(".", 1)[0].split(".") | ||
| dirpath = "/".join(parts) | ||
| granularity = get_granularity(parts) | ||
|
|
||
| stack_root = "/".join(parts[:granularity]) + "/" | ||
| file_path = f"{dirpath}/{abs_path}" | ||
| return stack_root, file_path | ||
|
|
||
|
|
||
| def get_granularity(parts: Sequence[str]) -> int: | ||
| # a.Bar, Bar.kt -> stack_root: a/, file_path: a/Bar.kt | ||
| granularity = 1 | ||
|
|
||
|
|
@@ -163,6 +172,10 @@ def get_path_from_module(module: str, abs_path: str) -> tuple[str, str]: | |
| # file_path: uk/co/example/foo/bar/Baz.kt | ||
| granularity = STACK_ROOT_MAX_LEVEL | ||
|
|
||
| stack_root = "/".join(parts[:granularity]) + "/" | ||
| file_path = f"{dirpath}/{abs_path}" | ||
| return stack_root, file_path | ||
| elif options.get("auto_source_code_config.multi_module_java"): | ||
| # com.example.multi.foo.bar.Baz$InnerClass, Baz.kt -> | ||
| # stack_root: com/example/multi/foo/ | ||
| # file_path: com/example/multi/foo/bar/Baz.kt | ||
| granularity = STACK_ROOT_MAX_LEVEL | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hm, just wondering, if there are second level TLDs would this still work? E.g.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have filed it as a ticket to follow-up on.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: TLD Check Conflicts with Java Multi-Module OptionThe
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's fine. |
||
|
|
||
| return granularity | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -176,11 +176,11 @@ def _process_and_assert_configuration_changes( | |
| expected_new_code_mappings | ||
| ) | ||
| for expected_cm in expected_new_code_mappings: | ||
| code_mapping = current_code_mappings.filter( | ||
| stack_root=expected_cm["stack_root"], | ||
| source_root=expected_cm["source_root"], | ||
| ).first() | ||
| code_mapping = current_code_mappings.get( | ||
| project_id=self.project.id, stack_root=expected_cm["stack_root"] | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The index together is the project and the stack root. |
||
| ) | ||
| assert code_mapping is not None | ||
| assert code_mapping.source_root == expected_cm["source_root"] | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When a test fails, this assertion is better than when source_root did not match (the call to |
||
| assert code_mapping.repository.name == expected_cm["repo_name"] | ||
| else: | ||
| assert current_code_mappings.count() == starting_code_mappings_count | ||
|
|
@@ -1003,3 +1003,64 @@ def test_categorized_frames_are_not_processed(self) -> None: | |
| expected_new_code_mappings=[self.code_mapping("android/app/", "src/android/app/")], | ||
| expected_new_in_app_stack_trace_rules=["stack.module:android.app.** +app"], | ||
| ) | ||
|
|
||
| def test_multi_module_with_old_granularity(self) -> None: | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the current behaviour without the option. |
||
| java_module_prefix = "com.example.multi" | ||
| module_prefix = java_module_prefix.replace(".", "/") + "/" | ||
| repo_trees = { | ||
| REPO1: [ | ||
| f"modules/modX/{module_prefix}foo/Bar.kt", | ||
| f"modules/modY/{module_prefix}bar/Baz.kt", | ||
| ] | ||
| } | ||
| frames = [ | ||
| self.frame_from_module(f"{java_module_prefix}.foo.Bar", "Bar.kt"), | ||
| self.frame_from_module(f"{java_module_prefix}.bar.Baz", "Baz.kt"), | ||
| ] | ||
| self._process_and_assert_configuration_changes( | ||
| repo_trees=repo_trees, | ||
| frames=frames, | ||
| platform=self.platform, | ||
| expected_new_code_mappings=[ | ||
| # It's missing the extra granularity | ||
| # It's going to pick modY since it is the first frame processed, thus, | ||
| # the other frame will not have a working code mapping | ||
| self.code_mapping("com/example/multi/", "modules/modY/com/example/multi/") | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code mapping will not work for |
||
| ], | ||
| expected_new_in_app_stack_trace_rules=["stack.module:com.example.** +app"], | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has one degree of granularity than with the new feature. - stack.module:com.example.** +app
+ stack.module:com.example.multi.** +app |
||
| ) | ||
|
|
||
| def test_multi_module(self) -> None: | ||
| # Some Java projects have all modules under the same com/foo/bar directory | ||
| # however, some projects have different modules under different directories | ||
| # Case 1: | ||
| # com.example.multi.foo -> modules/com/example/multi/foo/Bar.kt | ||
| # com.example.multi.bar -> modules/com/example/multi/bar/Baz.kt | ||
| # Case 2: | ||
| # com.example.multi.foo -> modules/modX/com/example/multi/foo/Bar.kt (Notice modX infix) | ||
| # com.example.multi.bar -> modules/modY/com/example/multi/bar/Baz.kt (Notice modY infix) | ||
| java_module_prefix = "com.example.multi" | ||
| module_prefix = java_module_prefix.replace(".", "/") + "/" | ||
| repo_trees = { | ||
| REPO1: [ | ||
| f"modules/modX/{module_prefix}foo/Bar.kt", | ||
| f"modules/modY/{module_prefix}bar/Baz.kt", | ||
| ] | ||
| } | ||
| frames = [ | ||
| self.frame_from_module(f"{java_module_prefix}.foo.Bar", "Bar.kt"), | ||
| self.frame_from_module(f"{java_module_prefix}.bar.Baz", "Baz.kt"), | ||
| ] | ||
| with self.options({"auto_source_code_config.multi_module_java": True}): | ||
| self._process_and_assert_configuration_changes( | ||
| repo_trees=repo_trees, | ||
| frames=frames, | ||
| platform=self.platform, | ||
| expected_new_code_mappings=[ | ||
| self.code_mapping(f"{module_prefix}foo/", f"modules/modX/{module_prefix}foo/"), | ||
| self.code_mapping(f"{module_prefix}bar/", f"modules/modY/{module_prefix}bar/"), | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A code mapping per module rather than a single code mapping. |
||
| ], | ||
| expected_new_in_app_stack_trace_rules=[ | ||
| f"stack.module:{java_module_prefix}.** +app" | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has one granularity degree greater than the default behaviour. See previous test. |
||
| ], | ||
| ) | ||

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 believe this logic is correct, but I'm wondering if we can simplify it, e.g.:
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.
That probably makes sense. I will handle it on my next PR.