-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(derived_code_mappings): Support Java #86280
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
796467c
8128f38
a52edee
7306adb
06b4756
2c8d6a6
92fead9
06af5f1
1ad50f0
2c0f4ae
cf4b3ad
74457f5
78b2829
038c67d
60023e9
9f2020f
09708e9
7246cbd
f5f0352
e642e72
df769cf
7e0de9c
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 |
---|---|---|
|
@@ -12,6 +12,9 @@ | |
RepoTreesIntegration, | ||
get_extension, | ||
) | ||
from sentry.issues.auto_source_code_config.constants import ( | ||
EXTRACT_FILENAME_FROM_MODULE_AND_ABS_PATH, | ||
) | ||
from sentry.models.organization import Organization | ||
from sentry.models.project import Project | ||
from sentry.models.repository import Repository | ||
|
@@ -51,6 +54,14 @@ class NeedsExtension(Exception): | |
pass | ||
|
||
|
||
class MissingModuleOrAbsPath(Exception): | ||
pass | ||
|
||
|
||
class DoesNotFollowJavaPackageNamingConvention(Exception): | ||
pass | ||
|
||
|
||
def derive_code_mappings( | ||
organization: Organization, | ||
frame: Mapping[str, Any], | ||
|
@@ -73,7 +84,10 @@ def derive_code_mappings( | |
# XXX: Look at sentry.interfaces.stacktrace and maybe use that | ||
class FrameInfo: | ||
def __init__(self, frame: Mapping[str, Any], platform: str | None = None) -> None: | ||
# XXX: platform will be used in a following PR | ||
if platform in EXTRACT_FILENAME_FROM_MODULE_AND_ABS_PATH: | ||
self.frame_info_from_module(frame) | ||
return | ||
|
||
frame_file_path = frame["filename"] | ||
frame_file_path = self.transformations(frame_file_path) | ||
|
||
|
@@ -123,6 +137,15 @@ def transformations(self, frame_file_path: str) -> str: | |
|
||
return frame_file_path | ||
|
||
def frame_info_from_module(self, frame: Mapping[str, Any]) -> None: | ||
if frame.get("module") and frame.get("abs_path"): | ||
stack_root, filepath = get_path_from_module(frame["module"], frame["abs_path"]) | ||
self.stack_root = stack_root | ||
self.raw_path = filepath | ||
self.normalized_path = filepath | ||
else: | ||
raise MissingModuleOrAbsPath("Investigate why the data is missing.") | ||
|
||
def __repr__(self) -> str: | ||
return f"FrameInfo: {self.raw_path}" | ||
|
||
|
@@ -214,8 +237,12 @@ def _stacktrace_buckets( | |
buckets[frame_filename.stack_root].append(frame_filename) | ||
except UnsupportedFrameInfo: | ||
logger.warning("Frame's filepath not supported: %s", frame.get("filename")) | ||
except MissingModuleOrAbsPath: | ||
logger.warning("Do not panic. I'm collecting this data.") | ||
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. Once I review the data I will remove reporting it to Sentry. |
||
except NeedsExtension: | ||
logger.warning("Needs extension: %s", frame.get("filename")) | ||
except DoesNotFollowJavaPackageNamingConvention: | ||
pass | ||
except Exception: | ||
logger.exception("Unable to split stacktrace path into buckets") | ||
|
||
|
@@ -507,8 +534,10 @@ def find_roots(frame_filename: FrameInfo, source_path: str) -> tuple[str, str]: | |
return (stack_root, "") | ||
elif source_path.endswith(stack_path): # "Packaged" logic | ||
source_prefix = source_path.rpartition(stack_path)[0] | ||
package_dir = stack_path.split("/")[0] | ||
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. In Python, it has always been a single word representing the package name (e.g. |
||
return (f"{stack_root}{package_dir}/", f"{source_prefix}{package_dir}/") | ||
return ( | ||
f"{stack_root}{frame_filename.stack_root}/", | ||
f"{source_prefix}{frame_filename.stack_root}/", | ||
) | ||
Comment on lines
+538
to
+540
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. Any change to If it passes the tests, I think we should be fine, but I would keep a very close eye on related metrics after this PR lands. 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 been making sure the tests are sound. It has not required any changes to the Python packages. 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. Great! Apologies if I'm being a little overly paranoid 😅 |
||
elif stack_path.endswith(source_path): | ||
stack_prefix = stack_path.rpartition(source_path)[0] | ||
return (f"{stack_root}{stack_prefix}", "") | ||
|
@@ -541,3 +570,27 @@ def find_roots(frame_filename: FrameInfo, source_path: str) -> tuple[str, str]: | |
# validate_source_url should have ensured the file names match | ||
# so if we get here something went wrong and there is a bug | ||
raise UnexpectedPathException("Could not find common root from paths") | ||
|
||
|
||
# Based on # https://github.com/getsentry/symbolicator/blob/450f1d6a8c346405454505ed9ca87e08a6ff34b7/crates/symbolicator-proguard/src/symbolication.rs#L450-L485 | ||
def get_path_from_module(module: str, abs_path: str) -> tuple[str, str]: | ||
"""This attempts to generate a modified module and a real path from a Java module name and filename. | ||
Returns a tuple of (stack_root, source_path). | ||
""" | ||
# An `abs_path` is valid if it contains a `.` and doesn't contain a `$`. | ||
if "$" in abs_path or "." not in abs_path: | ||
# Split the module at the first '$' character and take the part before it | ||
# If there's no '$', use the entire module | ||
file_path = module.split("$", 1)[0] if "$" in module else module | ||
stack_root = module.rsplit(".", 1)[0].replace(".", "/") | ||
return stack_root, file_path.replace(".", "/") | ||
|
||
if "." not in module: | ||
raise DoesNotFollowJavaPackageNamingConvention | ||
|
||
# If module has a dot, take everything before the last dot | ||
# com.example.foo.Bar$InnerClass -> com/example/foo/ | ||
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. Looks like the code below doesn't have a trailing 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. Good call; thanks! |
||
stack_root = module.rsplit(".", 1)[0].replace(".", "/") | ||
file_path = f"{stack_root}/{abs_path}" | ||
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. In symbolicator we use only the part of 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 would not work for us either. I made up these two cases which are not going to help in deriving code mappings. However, we have many other cases that would help, thus, it's okay if these don't work.
|
||
|
||
return stack_root, file_path |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,15 @@ | ||
from .constants import DRY_RUN_PLATFORMS, SUPPORTED_LANGUAGES | ||
from sentry import options | ||
|
||
from .constants import SUPPORTED_LANGUAGES | ||
|
||
def supported_platform(platform: str | None) -> bool: | ||
return (platform or "") in SUPPORTED_LANGUAGES + DRY_RUN_PLATFORMS | ||
|
||
def supported_platform(platform: str | None = None) -> bool: | ||
return platform in SUPPORTED_LANGUAGES + dry_run_platforms() | ||
|
||
|
||
def dry_run_platforms() -> list[str]: | ||
return options.get("issues.auto_source_code_config.dry-run-platforms") | ||
|
||
MichaelSun48 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
def is_dry_run_platform(platform: str | None = None) -> bool: | ||
return platform in dry_run_platforms() |
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.
@AbhiPrasad @lcian does it look good to you?
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.
Yep.
I've found that for Groovy there's also a few more possible extensions:
.gvy
,.gy
,.gsh
even though this is not very important considering the usage of groovy etc.Sorce: https://blog.mrhaki.com/2011/10/groovy-goodness-default-groovy-script.html
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.
we should split up this constant to categorize by language, makes it a bit easier to review.
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 intend to refactor this.