load multiple debug-metadata.properties during runtime #3024

lbloder · 2023-11-06T07:53:12Z

📜 Description

Load all debug-metadata.properties and apply to SentryOptions.
Only the first debug-metadata.properties is taken into consideration for adding the Proguard UUID

💡 Motivation and Context

Fixes #2994

💚 How did you test it?

📝 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

# Conflicts: # sentry/src/main/java/io/sentry/internal/debugmeta/ResourcesDebugMetaLoader.java

github-actions · 2023-11-06T07:54:19Z

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

Generated by 🚫 dangerJS against 12eb43a

github-actions · 2023-11-06T08:09:59Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	396.58 ms	485.60 ms	89.02 ms
Size	1.72 MiB	2.27 MiB	554.95 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`b172d4e`	352.92 ms	446.50 ms	93.58 ms
`0bd723b`	375.20 ms	452.41 ms	77.20 ms
`283d83e`	416.81 ms	497.22 ms	80.41 ms
`283d83e`	348.44 ms	392.06 ms	43.62 ms
`0bd723b`	395.44 ms	472.66 ms	77.22 ms
`4e29063`	384.14 ms	447.74 ms	63.60 ms
`93a76ca`	391.54 ms	475.65 ms	84.11 ms
`0bf143e`	368.35 ms	437.47 ms	69.12 ms
`4e29063`	364.08 ms	445.51 ms	81.43 ms
`4bf95dd`	345.96 ms	414.24 ms	68.28 ms

App size

Revision	Plain	With Sentry	Diff
`b172d4e`	1.72 MiB	2.29 MiB	578.09 KiB
`0bd723b`	1.72 MiB	2.29 MiB	578.09 KiB
`283d83e`	1.72 MiB	2.29 MiB	577.69 KiB
`283d83e`	1.72 MiB	2.29 MiB	577.69 KiB
`0bd723b`	1.72 MiB	2.29 MiB	578.09 KiB
`4e29063`	1.72 MiB	2.29 MiB	578.38 KiB
`93a76ca`	1.72 MiB	2.29 MiB	576.75 KiB
`0bf143e`	1.72 MiB	2.29 MiB	576.50 KiB
`4e29063`	1.72 MiB	2.29 MiB	578.38 KiB
`4bf95dd`	1.72 MiB	2.29 MiB	576.40 KiB

Previous results on branch: feat/multiple-debug-meta.properties

Startup times

Revision	Plain	With Sentry	Diff
`bb69afa`	420.90 ms	486.08 ms	65.18 ms
`b5d05a5`	368.24 ms	445.90 ms	77.66 ms
`b950cf5`	422.09 ms	508.16 ms	86.06 ms
`943fdf8`	402.53 ms	480.56 ms	78.03 ms
`b87b12a`	379.98 ms	469.46 ms	89.48 ms

App size

Revision	Plain	With Sentry	Diff
`bb69afa`	1.72 MiB	2.29 MiB	577.37 KiB
`b5d05a5`	1.72 MiB	2.27 MiB	555.00 KiB
`b950cf5`	1.72 MiB	2.29 MiB	577.37 KiB
`943fdf8`	1.72 MiB	2.29 MiB	577.52 KiB
`b87b12a`	1.72 MiB	2.29 MiB	577.37 KiB

codecov · 2023-11-06T08:11:20Z

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Files	Coverage Δ
...core/internal/debugmeta/AssetsDebugMetaLoader.java	`88.88% <100.00%> (ø)`
sentry/src/main/java/io/sentry/Sentry.java	`85.95% <100.00%> (ø)`
...sentry/internal/debugmeta/NoOpDebugMetaLoader.java	`66.66% <ø> (ø)`
...y/internal/debugmeta/ResourcesDebugMetaLoader.java	`90.00% <100.00%> (+20.43%)`	⬆️
...ava/io/sentry/util/DebugMetaPropertiesApplier.java	`81.81% <80.00%> (+0.86%)`	⬆️
...karta/SentrySpringServletContainerInitializer.java	`0.00% <0.00%> (ø)`

📢 Thoughts on this report? Let us know!

# Conflicts: # CHANGELOG.md

adinauer

Left some remarks. Most important is stream handling and exception handling.

sentry/src/main/java/io/sentry/internal/debugmeta/ResourcesDebugMetaLoader.java

adinauer · 2023-11-07T08:56:49Z

sentry/src/main/java/io/sentry/internal/debugmeta/ResourcesDebugMetaLoader.java

    } catch (IOException e) {
      logger.log(SentryLevel.ERROR, e, "Failed to load %s", DEBUG_META_PROPERTIES_FILENAME);
+    } catch (RuntimeException e) {


h Should we move this into the while so we keep loading the files that do work?

adinauer · 2023-11-07T09:09:55Z

sentry/src/main/java/io/sentry/internal/debugmeta/ResourcesDebugMetaLoader.java

+      while (resourceUrls.hasMoreElements()) {
+        URL currenturl = resourceUrls.nextElement();
+        Properties currentProperties = new Properties();
+        currentProperties.load(currenturl.openStream());


h Stream should be closed, e.g. try(...) { ...

adinauer · 2023-11-07T09:14:10Z

sentry/src/main/java/io/sentry/util/DebugMetaPropertiesApplier.java

      final @Nullable String proguardUuid =
-          debugMetaProperties.getProperty("io.sentry.ProguardUuids");
+          debugMetaProperties.get(0).getProperty("io.sentry.ProguardUuids");


m I presume this is fine as there should never be more than one debug-meta.properties on Android but I guess it'd be safer to iterate over the list until we find a proguard ID. Would still be first one wins but I guess that's OK, can always improve later.

adinauer

LGTM, just needs conflicts resolved

# Conflicts: # CHANGELOG.md # sentry/api/sentry.api # sentry/src/main/java/io/sentry/util/DebugMetaPropertiesApplier.java

# Conflicts: # CHANGELOG.md

lbloder added 3 commits October 27, 2023 07:02

load multiple debug-metadata.properties during runtime

6638d30

apply proguard uuid of first properties

329a9f8

Merge branch 'main' into feat/multiple-debug-meta.properties

de4b1f1

# Conflicts: # sentry/src/main/java/io/sentry/internal/debugmeta/ResourcesDebugMetaLoader.java

add changelog

08d19a8

Merge branch 'main' into feat/multiple-debug-meta.properties

6a39fd2

# Conflicts: # CHANGELOG.md

adinauer reviewed Nov 7, 2023

View reviewed changes

lbloder added 2 commits November 7, 2023 14:13

code review changes

054732c

Merge branch 'main' into feat/multiple-debug-meta.properties

cce539c

lbloder marked this pull request as ready for review November 7, 2023 15:39

lbloder requested review from romtsn, stefanosiano and markushi as code owners November 7, 2023 15:39

adinauer approved these changes Nov 29, 2023

View reviewed changes

lbloder added 2 commits November 29, 2023 16:59

Merge branch 'main' into feat/multiple-debug-meta.properties

e17b0af

# Conflicts: # CHANGELOG.md # sentry/api/sentry.api # sentry/src/main/java/io/sentry/util/DebugMetaPropertiesApplier.java

Merge branch 'main' into feat/multiple-debug-meta.properties

12eb43a

# Conflicts: # CHANGELOG.md

lbloder merged commit ee349f9 into main Dec 4, 2023
19 checks passed

lbloder deleted the feat/multiple-debug-meta.properties branch December 4, 2023 09:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

load multiple debug-metadata.properties during runtime #3024

load multiple debug-metadata.properties during runtime #3024

lbloder commented Nov 6, 2023

github-actions bot commented Nov 6, 2023 •

edited

github-actions bot commented Nov 6, 2023 •

edited

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/multiple-debug-meta.properties

Startup times

App size

codecov bot commented Nov 6, 2023 •

edited

adinauer left a comment

adinauer Nov 7, 2023

adinauer Nov 7, 2023

adinauer Nov 7, 2023

adinauer left a comment

load multiple debug-metadata.properties during runtime #3024

load multiple debug-metadata.properties during runtime #3024

Conversation

lbloder commented Nov 6, 2023

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Nov 6, 2023 • edited

github-actions bot commented Nov 6, 2023 • edited

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/multiple-debug-meta.properties

Startup times

App size

codecov bot commented Nov 6, 2023 • edited

Codecov Report

adinauer left a comment

Choose a reason for hiding this comment

adinauer Nov 7, 2023

Choose a reason for hiding this comment

adinauer Nov 7, 2023

Choose a reason for hiding this comment

adinauer Nov 7, 2023

Choose a reason for hiding this comment

adinauer left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 6, 2023 •

edited

github-actions bot commented Nov 6, 2023 •

edited

codecov bot commented Nov 6, 2023 •

edited