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

feat: move plugin from sentry_dart_plugin (experimental) #1099

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vaind
Copy link
Collaborator

@vaind vaind commented Nov 2, 2022

📜 Description

Testing whether moving plugin directly to the sentry package has any impact on app size or usability.

The way it's added in this PR, users would be able to run plugin by dart run sentry:plugin

TODO:

  • I don't like the dependency updates - try if it can somehow be integrated as a dev_dependency instead - maybe if the actual source is kept as a separate package?

💡 Motivation and Context

See discussions here: getsentry/sentry-dart-plugin#64

💚 How did you test it?

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

Fails
🚫 Please consider adding a changelog entry for the next release.
Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- move plugin from sentry_dart_plugin (experimental) ([#1099](https://github.com/getsentry/sentry-dart/pull/1099))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 66576c9

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 273.25 ms 320.21 ms 46.96 ms
Size 5.94 MiB 6.95 MiB 1.01 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
1c6eb5b 350.69 ms 393.86 ms 43.17 ms
af2d175 279.08 ms 312.37 ms 33.29 ms
9c5aec6 287.33 ms 320.94 ms 33.61 ms
633cf2e 289.36 ms 340.38 ms 51.02 ms
21845e2 345.08 ms 382.82 ms 37.74 ms
6d317ea 303.46 ms 356.06 ms 52.60 ms
04db237 330.16 ms 428.38 ms 98.22 ms
3e9fb0e 329.14 ms 359.16 ms 30.02 ms
559d28f 302.35 ms 339.53 ms 37.18 ms
eecbbca 324.37 ms 352.49 ms 28.12 ms

App size

Revision Plain With Sentry Diff
1c6eb5b 5.94 MiB 6.92 MiB 1001.53 KiB
af2d175 5.94 MiB 6.92 MiB 1001.83 KiB
9c5aec6 5.94 MiB 6.92 MiB 1001.61 KiB
633cf2e 5.94 MiB 6.92 MiB 1001.53 KiB
21845e2 5.94 MiB 6.92 MiB 1003.77 KiB
6d317ea 5.94 MiB 6.92 MiB 1001.74 KiB
04db237 5.94 MiB 6.95 MiB 1.01 MiB
3e9fb0e 5.94 MiB 6.95 MiB 1.01 MiB
559d28f 5.94 MiB 6.92 MiB 1001.70 KiB
eecbbca 5.94 MiB 6.89 MiB 975.78 KiB

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1280.07 ms 1309.82 ms 29.75 ms
Size 8.15 MiB 9.13 MiB 994.20 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
6d317ea 1277.27 ms 1287.47 ms 10.20 ms
613760b 1263.10 ms 1277.27 ms 14.16 ms
1c6eb5b 1277.85 ms 1285.71 ms 7.86 ms
2f8f173 1280.61 ms 1292.20 ms 11.59 ms
eecbbca 1264.90 ms 1286.33 ms 21.43 ms
559d28f 1265.04 ms 1288.96 ms 23.92 ms
56810ff 1267.59 ms 1293.48 ms 25.89 ms
d7758e8 1271.69 ms 1288.08 ms 16.39 ms
4efee31 1270.33 ms 1285.75 ms 15.42 ms
eb1a7c1 1281.25 ms 1295.40 ms 14.15 ms

App size

Revision Plain With Sentry Diff
6d317ea 8.15 MiB 9.12 MiB 986.26 KiB
613760b 8.15 MiB 9.13 MiB 1000.46 KiB
1c6eb5b 8.15 MiB 9.12 MiB 986.27 KiB
2f8f173 8.15 MiB 9.13 MiB 1000.39 KiB
eecbbca 8.15 MiB 9.10 MiB 965.26 KiB
559d28f 8.15 MiB 9.12 MiB 987.32 KiB
56810ff 8.15 MiB 9.12 MiB 987.35 KiB
d7758e8 8.15 MiB 9.12 MiB 989.76 KiB
4efee31 8.15 MiB 9.12 MiB 991.35 KiB
eb1a7c1 8.15 MiB 9.13 MiB 1000.10 KiB

@marandaneto
Copy link
Contributor

@vaind I meant to raise an issue only, I still have to discuss with folks if this is the way to go.
The Android Gradle plugin, for example, was a monorepo, and this only caused problems.
So we've to point out the trade-offs before going ahead, let's pause this before we are sure that we want to do it.

@vaind
Copy link
Collaborator Author

vaind commented Nov 3, 2022

@vaind I meant to raise an issue only, I still have to discuss with folks if this is the way to go. The Android Gradle plugin, for example, was a monorepo, and this only caused problems. So we've to point out the trade-offs before going ahead, let's pause this before we are sure that we want to do it.

I know, I've only done this in order to see the impact on the size, as measured by the SDK-metrics CI - it was a quick thing to do because I just moved the code from the other repo.

This way we can actually know what happens rather than trying to guess whether the app size is affected or not (and how much), which may save a lot of talking with the actual data in hand :)

@getsantry
Copy link

getsantry bot commented Oct 18, 2023

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added the Stale label Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants