Tracing for File IO integration#1160
Conversation
| pub:sentry_flutter: | ||
| pub:sentry_logging: | ||
| pub:sentry_dio: | ||
| #pub:sentry_dio: |
There was a problem hiding this comment.
Double check if craft is already able to auto create the first entry in release registry
There was a problem hiding this comment.
@adinauer @kamilogorek what's the outcome of that problem, do we still need to do the release registry dance manually or is it fine now?
getsentry/sentry-release-registry#85
There was a problem hiding this comment.
Yes, it's still "broken"
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1160 +/- ##
==========================================
- Coverage 90.14% 88.87% -1.28%
==========================================
Files 119 121 +2
Lines 3672 3810 +138
==========================================
+ Hits 3310 3386 +76
- Misses 362 424 +62 ☔ View full report in Codecov by Sentry. |
| /// | ||
| /// All the copy, create, delete, open, rename, read, and write operations are | ||
| /// supported. | ||
| File sentryTrace({ |
There was a problem hiding this comment.
What's the reason to not just automatically enable it for all file io?
You can do that with the following code:
IOOverrides.global = SentryIoOverrides(HubAdapter(), options);
File f = File('path/to/file'); // file is SentryFile by default
class SentryIoOverrides extends IOOverrides {
final Hub _hub;
final SentryOptions _options;
SentryIoOverrides(this._hub, this._options);
@override
File createFile(String path) {
if (!_options.isTracingEnabled()) {
return super.createFile(path);
}
return SentryFile(
super.createFile(path),
_hub,
_options,
);
}
}There was a problem hiding this comment.
I did see that and it is indeed awesome, but its also misusing the mocks API, as an SDK, we need to be a bit more conservative about that, I guess.
I'm afraid that this could break the app's behavior that relies on the File type or something like that, or, that some files take more time than usual and you don't want to trace them for some reason (overhead, long transactions, etc), an all in solution could be a deal breaker for some users.
I'd prefer to do this next after gathering some feedback on the current implementation.
What do you think?
There was a problem hiding this comment.
Yeah, that sounds reasonable
There was a problem hiding this comment.
Yep, I believe it still makes sense to have it, its a hook that makes it possible since Dart does not have reflection nor monkey patching, not sure if it'd be ever enabled by default tho.
At least not during the first version.
|
@krystofwoldrich @brustolin this is ready for review |
|
|
||
| data = callback() as T; | ||
|
|
||
| if (!hasLength) { |
There was a problem hiding this comment.
L: What if we are overriding the content of the file do we want to know the length before or after?
If I'm looking correctly at the moment we would get the length before.
There was a problem hiding this comment.
The data can only be read before, because after, the file can be deleted, renamed, moved, so it's a trade-off we made.
Maybe I should point that out in the docs.
Android Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| eecbbca | 324.37 ms | 352.49 ms | 28.12 ms |
| cdf7172 | 348.54 ms | 390.81 ms | 42.27 ms |
| 2331d89 | 352.45 ms | 417.34 ms | 64.89 ms |
| 559d28f | 302.35 ms | 339.53 ms | 37.18 ms |
| 453e1bc | 320.41 ms | 372.73 ms | 52.32 ms |
| f922f8f | 332.31 ms | 374.67 ms | 42.37 ms |
| 49a149b | 301.96 ms | 344.51 ms | 42.55 ms |
| 9f9f94f | 331.04 ms | 368.92 ms | 37.88 ms |
| abcdba3 | 354.68 ms | 399.04 ms | 44.36 ms |
| 5603ab2 | 309.84 ms | 345.20 ms | 35.36 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| eecbbca | 5.94 MiB | 6.89 MiB | 975.78 KiB |
| cdf7172 | 5.94 MiB | 6.95 MiB | 1.01 MiB |
| 2331d89 | 5.94 MiB | 6.96 MiB | 1.02 MiB |
| 559d28f | 5.94 MiB | 6.92 MiB | 1001.70 KiB |
| 453e1bc | 5.94 MiB | 6.95 MiB | 1.01 MiB |
| f922f8f | 5.94 MiB | 6.95 MiB | 1.01 MiB |
| 49a149b | 5.94 MiB | 6.92 MiB | 1001.54 KiB |
| 9f9f94f | 5.94 MiB | 6.95 MiB | 1.01 MiB |
| abcdba3 | 5.94 MiB | 6.95 MiB | 1.01 MiB |
| 5603ab2 | 5.94 MiB | 6.95 MiB | 1.01 MiB |
iOS Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 04db237 | 1273.29 ms | 1306.50 ms | 33.21 ms |
| abcdba3 | 1257.31 ms | 1283.49 ms | 26.18 ms |
| 2331d89 | 1260.86 ms | 1281.24 ms | 20.39 ms |
| 0db91cc | 1267.63 ms | 1279.69 ms | 12.06 ms |
| ef2f368 | 1259.12 ms | 1277.04 ms | 17.92 ms |
| 21845e2 | 1279.37 ms | 1298.81 ms | 19.45 ms |
| aa950e9 | 1275.17 ms | 1295.33 ms | 20.16 ms |
| 9f9f94f | 1268.33 ms | 1284.73 ms | 16.41 ms |
| ccc09e4 | 1254.74 ms | 1277.08 ms | 22.34 ms |
| ae02632 | 1286.77 ms | 1300.37 ms | 13.60 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 04db237 | 8.15 MiB | 9.13 MiB | 1003.16 KiB |
| abcdba3 | 8.15 MiB | 9.12 MiB | 989.76 KiB |
| 2331d89 | 8.16 MiB | 9.17 MiB | 1.01 MiB |
| 0db91cc | 8.15 MiB | 9.15 MiB | 1018.56 KiB |
| ef2f368 | 8.15 MiB | 9.10 MiB | 965.24 KiB |
| 21845e2 | 8.15 MiB | 9.12 MiB | 991.34 KiB |
| aa950e9 | 8.16 MiB | 9.17 MiB | 1.01 MiB |
| 9f9f94f | 8.15 MiB | 9.15 MiB | 1020.76 KiB |
| ccc09e4 | 8.16 MiB | 9.16 MiB | 1.01 MiB |
| ae02632 | 8.16 MiB | 9.15 MiB | 1020.68 KiB |
📜 Description
Tracing for File IO integration
💡 Motivation and Context
Closes #835
💚 How did you test it?
📝 Checklist
🔮 Next steps