feat(auto_source): Derive a code mapping per module #101288
Conversation
The work to derive code mappings for Java fell short of supporting multi module directory structure. This adds support for multi module directory structure behind a feature flag.
github-actions
bot
added
the
Scope: Backend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| # 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 |
There was a problem hiding this comment.
The default behaviour is STACK_ROOT_MAX_LEVEL - 1
I will need to investigate other projects where code mappings did not fully work and probably make this the new default.
There was a problem hiding this comment.
hm, just wondering, if there are second level TLDs would this still work? E.g. au/com/company/app/feature/ ? Found this repo as an exemplar: https://github.com/Automattic/pocket-casts-android/tree/main/modules/features/account/src/main/java/au/com/shiftyjelly/pocketcasts/account
There was a problem hiding this comment.
I have filed it as a ticket to follow-up on.
| source_root=expected_cm["source_root"], | ||
| ).first() | ||
| code_mapping = current_code_mappings.get( | ||
| project_id=self.project.id, stack_root=expected_cm["stack_root"] |
There was a problem hiding this comment.
The index together is the project and the stack root.
| project_id=self.project.id, stack_root=expected_cm["stack_root"] | ||
| ) | ||
| assert code_mapping is not None | ||
| assert code_mapping.source_root == expected_cm["source_root"] |
There was a problem hiding this comment.
When a test fails, this assertion is better than when source_root did not match (the call to first() would give us None).
| expected_new_in_app_stack_trace_rules=["stack.module:android.app.** +app"], | ||
| ) | ||
|
|
||
| def test_multi_module_with_old_granularity(self) -> None: |
There was a problem hiding this comment.
This is the current behaviour without the option.
We will delete this test once we're fully rolled out.
| # 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/") |
There was a problem hiding this comment.
This code mapping will not work for com.example.multi.foo.Bar since the file is found under modules/modX/com.example.multi/foo/Bar.kt.
| # the other frame will not have a working code mapping | ||
| self.code_mapping("com/example/multi/", "modules/modY/com/example/multi/") | ||
| ], | ||
| expected_new_in_app_stack_trace_rules=["stack.module:com.example.** +app"], |
There was a problem hiding this comment.
This has one degree of granularity than with the new feature.
- stack.module:com.example.** +app
+ stack.module:com.example.multi.** +app| 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/"), |
There was a problem hiding this comment.
A code mapping per module rather than a single code mapping.
| self.code_mapping(f"{module_prefix}bar/", f"modules/modY/{module_prefix}bar/"), | ||
| ], | ||
| expected_new_in_app_stack_trace_rules=[ | ||
| f"stack.module:{java_module_prefix}.** +app" |
There was a problem hiding this comment.
This has one granularity degree greater than the default behaviour. See previous test.
armenzg
changed the title
| return stack_root, file_path | ||
|
|
||
|
|
||
| def get_granularity(parts: Sequence[str]) -> int: |
There was a problem hiding this comment.
I believe this logic is correct, but I'm wondering if we can simplify it, e.g.:
num_segments = len(parts)
if num_segments == 1:
return 1
# com.example.foo.bar.Baz$InnerClass, Baz.kt ->
# stack_root: com/example/foo/
# file_path: com/example/foo/bar/Baz.kt
granularity = STACK_ROOT_MAX_LEVEL - 1
# com.example.foo.bar.Baz$InnerClass, Baz.kt ->
# stack_root: com/example/foo/
# file_path: com/example/foo/bar/Baz.kt
if parts[1] in SECOND_LEVEL_TLDS:
granularity = STACK_ROOT_MAX_LEVEL
# com.example.multi.foo.bar.Baz$InnerClass, Baz.kt ->
# stack_root: com/example/multi/foo/
# file_path: com/example/multi/foo/bar/Baz.kt
elif options.get("auto_source_code_config.multi_module_projects_allowlist"):
granularity = STACK_ROOT_MAX_LEVEL
return granularity
There was a problem hiding this comment.
That probably makes sense. I will handle it on my next PR.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
armenzg
force-pushed
the
0/feat/auto_source/java/multi_module/armenzg
branch
from
7194ad3 to
4d7c7c1
Compare
|
bugbot run |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #101288 +/- ##
===========================================
+ Coverage 81.04% 81.08% +0.03%
===========================================
Files 8681 8683 +2
Lines 385115 385368 +253
Branches 24320 24320
===========================================
+ Hits 312132 312461 +329
+ Misses 72624 72548 -76
Partials 359 359 |
Remove option and old code path from #101288.
This makes the granularity of the stack root consistent rather than trying to optimize number of created code mappings. This also removes the option and old code path from #101288.
The work to derive code mappings for Java fell short of supporting multi module directory structure. This adds support for multi module directory structure behind a feature flag.
This makes the granularity of the stack root consistent rather than trying to optimize number of created code mappings. This also removes the option and old code path from #101288.
The work to derive code mappings for Java fell short of supporting multi module directory structure.
This adds support for multi module directory structure behind a feature flag.