Skip to content
This repository has been archived by the owner on Aug 30, 2023. It is now read-only.

fix: do not lookup sentry-debug-meta but instead load it directly #445

Merged
merged 6 commits into from
Jun 3, 2020
Merged

fix: do not lookup sentry-debug-meta but instead load it directly #445

merged 6 commits into from
Jun 3, 2020

Conversation

marandaneto
Copy link
Contributor

@marandaneto marandaneto commented Jun 3, 2020

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

fix: do not lookup sentry-debug-meta but instead loads it directly

💡 Motivation and Context

probably fixes #442
one may have thousands of asset files and this will slow down the startup time.
instead of looking up if the sentry-debug-meta exists, we try to load it directly and take care of FileNotFoundException.
the downside is: if one doesn't have asset files, catching the exception might be more expensive than the looking up itself, but worth it.

💚 How did you test it?

running it, tests are in place.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing

🔮 Next steps

ideally sentry-debug-meta.properties will be a manifest config or SentryOptions, as a file this is suboptimal, but needed because of retro compatibility.

@marandaneto marandaneto changed the title fix/slow assets manager fix: do not lookup sentry-debug-meta but instead loads it directly Jun 3, 2020
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

All minor comments except for two important ones.
Good bet with just opening the asset and not checking if it exists. I guess the downside with catching the exception is not going to slow us down that much. I think it is a good tradeoff.

@philipphofmann philipphofmann changed the title fix: do not lookup sentry-debug-meta but instead loads it directly fix: do not lookup sentry-debug-meta but instead load it directly Jun 3, 2020
Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

LGTM

@marandaneto marandaneto merged commit 3a6dc09 into getsentry:master Jun 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

App takes much longer to load when App is built with sentry sdk
3 participants