Add Java Source context to stack frames #46497
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #46497 +/- ##
=======================================
Coverage 80.73% 80.74%
=======================================
Files 4761 4761
Lines 201288 201386 +98
Branches 11626 11626
=======================================
+ Hits 162512 162608 +96
- Misses 38518 38520 +2
Partials 258 258
|
|
This seems like stack trace processing so requesting a review from the processing team, as they own this part. |
src/sentry/lang/java/plugin.py
Outdated
| # TODO unable to use dif cache as file can't be recognized as ZIP by ArtifactBundleArchive(file) | ||
| difs = ProjectDebugFile.objects.find_by_debug_ids(self.project, self.images) | ||
|
|
||
| for new_frame in new_frames: | ||
| lineno = new_frame.get("lineno") | ||
| if not lineno: | ||
| continue | ||
|
|
||
| source_file_name = self._build_source_file_name(new_frame) | ||
|
|
||
| for key, dif in difs.items(): | ||
| file = dif.file.getfile(prefetch=True) | ||
| archive = ArtifactBundleArchive(file) |
There was a problem hiding this comment.
we do a database query for every frame, and then open up the ArtifactBundleArchive for at least every frame (plus, every inlined frame and every debug-id).
We should definitely do this in the constructor of this class, or lazily on-demand.
Are we guaranteed to only have a single debug-id by event? What would happen if we have more than one?
There was a problem hiding this comment.
We should definitely do this in the constructor of this class, or lazily on-demand.
Moved it to init
Are we guaranteed to only have a single debug-id by event? What would happen if we have more than one?
It should try all of them until it's able to find the source file. Can add a test for this case.
There was a problem hiding this comment.
Are we guaranteed to only have a single debug-id by event?
Not there could be multiple per event.
src/sentry/lang/java/plugin.py
Outdated
|
|
||
| platform = frame.get("platform") or self.data.get("platform") | ||
| self._handles_frame = ( | ||
| platform == "java" and self.available and "module" in frame and "lineno" in frame |
There was a problem hiding this comment.
You are looking at the lineno of the raw frame, before proguard mapping. Which means this is out of sync with whatever frames you will actually process in process_frame.
There was a problem hiding this comment.
I assumed that even with proguard if there's no line number in the first place we'd be unable to produce one via proguard mapping. No lineno means no source lookup possible. Maybe my assumption is wrong. I can remove the lineno check. Though the check here doesn't mean we use the "raw" lineno for lookup. In case JavaStacktraceProcessor (doing proguard mapping) remaps it, we take the remapped lineno. This behaviour is also tested by the last test.
src/sentry/lang/java/plugin.py
Outdated
| @@ -138,16 +138,23 @@ def __init__(self, *args, **kwargs): | |||
| self._handles_frame = None | |||
| self.images = get_jvm_images(self.data) | |||
| self.available = len(self.images) > 0 | |||
| difs = ProjectDebugFile.objects.find_by_debug_ids(self.project, self.images) | |||
| self._archives = {} | |||
| for key, dif in difs.items(): | |||
There was a problem hiding this comment.
you never use the key anywhere, so I think you can just remove it and use a list instead of a dict.
There was a problem hiding this comment.
I think this is good and can bemerged IF:
- the Query
ProjectDebugFile.objects.find_by_debug_ids()is ok. (Read somewhere that because of hybrid cloud there a new API to use to query stuff, but I am not sure if this is a concern here. Someone with more knowledge of how all this works should be consulted about this) - it is OK to load one (or maybe multiple
ArtifactBundleArchive(file)into memory on start of theJavaSourceLookupStacktraceProcessor. Again someone with more knowledge about how this is run should be asked if there is a concern of using too much memory here.
I believe that is fine. The JS processor and lookup API is also loading a couple of bundles at the same time. |
…o list; other cleanup
Adds a new processor to the
JavaPlugincalledJavaSourceLookupStacktraceProcessorwhich looks up stack frames in available source bundles and tries to find the source code line plus some context lines.JavaSourceLookupStacktraceProcessorwraps the existingJavaStacktraceProcessoras only the first processor that handles a certain frame is called but we need both of them to run. In case of obfuscated sources,JavaStacktraceProcessordeobfuscates them (possibly exploding a single frame into multiple) then we look up sources inJavaSourceLookupStacktraceProcessor.We're using a new debug file type
jvm(see getsentry/relay#2002 and getsentry/sentry-cli#1551). This causes tests to fail because other PRs haven't been merged and released yet.getsentry/sentry-java#633