Skip to content
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

Detect dependencies by listing MANIFEST.MF files at runtime #2538

Merged
merged 8 commits into from
Feb 16, 2023

Conversation

adinauer
Copy link
Member

📜 Description

This allows us to collect dependencies at runtime by looking at all available MANIFEST.MF resources and extracting the dependency name and version from the URL. This does not actually look at the contents of the manifest files but only at the URLs which usually takes a low single digit amount of ms to collect all dependencies.

💡 Motivation and Context

Allows us to show modules in UI for JVM applications that aren't using our plugin to provide sentry-external-modules.txt. A workaround for #2474 until we can provide a non Android focused build plugin that users can use.

💚 How did you test it?

Running Spring Boot from IDE, from runnable JAR and WAR deployed to Tomcat. Unit Tests

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • [-] Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

@github-actions
Copy link
Contributor

github-actions bot commented Feb 14, 2023

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 17dee9e

@github-actions
Copy link
Contributor

github-actions bot commented Feb 14, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 315.92 ms 333.12 ms 17.20 ms
Size 1.73 MiB 2.34 MiB 625.56 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
4b32504 315.69 ms 373.96 ms 58.27 ms
754021c 358.70 ms 361.98 ms 3.28 ms
c1399c1 345.06 ms 385.49 ms 40.43 ms
5fa24ec 326.29 ms 384.53 ms 58.24 ms
fe30606 310.82 ms 335.36 ms 24.55 ms
f6a135d 263.96 ms 383.59 ms 119.63 ms
14c083a 350.82 ms 388.86 ms 38.04 ms
4b32504 357.14 ms 404.04 ms 46.90 ms
fe30606 327.46 ms 351.74 ms 24.28 ms

App size

Revision Plain With Sentry Diff
4b32504 1.73 MiB 2.34 MiB 623.74 KiB
754021c 1.73 MiB 2.33 MiB 623.06 KiB
c1399c1 1.73 MiB 2.33 MiB 620.61 KiB
5fa24ec 1.73 MiB 2.33 MiB 620.61 KiB
fe30606 1.73 MiB 2.34 MiB 623.74 KiB
f6a135d 1.73 MiB 2.33 MiB 623.10 KiB
14c083a 1.73 MiB 2.33 MiB 620.61 KiB
4b32504 1.73 MiB 2.34 MiB 623.74 KiB
fe30606 1.73 MiB 2.34 MiB 623.74 KiB

Previous results on branch: feat/detect-modules-for-backends-via-manifest

Startup times

Revision Plain With Sentry Diff
81219d3 354.68 ms 401.62 ms 46.94 ms
cdbc5bf 360.98 ms 420.70 ms 59.72 ms

App size

Revision Plain With Sentry Diff
81219d3 1.73 MiB 2.34 MiB 624.75 KiB
cdbc5bf 1.73 MiB 2.34 MiB 624.75 KiB

@codecov
Copy link

codecov bot commented Feb 14, 2023

Codecov Report

Base: 80.21% // Head: 80.27% // Increases project coverage by +0.06% 🎉

Coverage data is based on head (17dee9e) compared to base (319f191).
Patch coverage: 90.90% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2538      +/-   ##
============================================
+ Coverage     80.21%   80.27%   +0.06%     
- Complexity     3970     3986      +16     
============================================
  Files           324      327       +3     
  Lines         14952    15010      +58     
  Branches       1972     1977       +5     
============================================
+ Hits          11994    12050      +56     
  Misses         2183     2183              
- Partials        775      777       +2     
Impacted Files Coverage Δ
...src/main/java/io/sentry/util/ClassLoaderUtils.java 25.00% <25.00%> (ø)
...entry/internal/modules/CompositeModulesLoader.java 90.00% <90.00%> (ø)
...sentry/internal/modules/ManifestModulesLoader.java 95.45% <95.45%> (ø)
sentry/src/main/java/io/sentry/Sentry.java 56.08% <100.00%> (+0.94%) ⬆️
...va/io/sentry/config/ClasspathPropertiesLoader.java 94.44% <100.00%> (+9.44%) ⬆️
...entry/internal/modules/ResourcesModulesLoader.java 80.00% <100.00%> (+9.41%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@adinauer adinauer changed the title Detect dependencies by looking at available MANIFEST.MF files Detect dependencies by listing MANIFEST.MF files at runtime Feb 14, 2023
if (modulesLoader instanceof NoOpModulesLoader) {
options.setModulesLoader(new ResourcesModulesLoader(options.getLogger()));
if (Platform.isJvm()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: I think we don't need this check, 'cause this was the codepath for JVM platforms anyway, so I think CompositeModulesLoader covers everything already

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK Great, I presume this also won't cause problems with other SDKs using the Java SDK, correct?

@Override
protected Map<String, String> loadModules() {
final @NotNull Map<String, String> modules = new HashMap<>();
List<Module> detectedModules = detectModulesViaManifestFiles();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: one minor question - are the modules alphabetically sorted within the MANIFEST.MF file? If not, I guess we'd have to sort them, not sure if there's sorting of those somewhere on relay or frontend.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're not sorted when sending the JSON but show up sorted in the UI. Guess that's fine then?

@adinauer adinauer merged commit d5473b8 into main Feb 16, 2023
@adinauer adinauer deleted the feat/detect-modules-for-backends-via-manifest branch February 16, 2023 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants