Skip to content

Tracing for File IO integration#1160

Merged
marandaneto merged 24 commits into
mainfrom
feat/sentry_file
Dec 6, 2022
Merged

Tracing for File IO integration#1160
marandaneto merged 24 commits into
mainfrom
feat/sentry_file

Conversation

@marandaneto

@marandaneto marandaneto commented Dec 2, 2022

Copy link
Copy Markdown
Contributor

📜 Description

Tracing for File IO integration

💡 Motivation and Context

Closes #835

💚 How did you test it?

screenshot_2022-12-05_at_13 35 47

📝 Checklist

🔮 Next steps

Comment thread .craft.yml Outdated
pub:sentry_flutter:
pub:sentry_logging:
pub:sentry_dio:
#pub:sentry_dio:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Double check if craft is already able to auto create the first entry in release registry

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, it's still "broken"

@github-actions

github-actions Bot commented Dec 2, 2022

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

Generated by 🚫 dangerJS against de0ca3c

@codecov-commenter

codecov-commenter commented Dec 2, 2022

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 55.07246% with 62 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.87%. Comparing base (62dde43) to head (de0ca3c).
⚠️ Report is 1521 commits behind head on main.

Files with missing lines Patch % Lines
file/lib/src/sentry_file.dart 54.13% 61 Missing ⚠️
file/lib/src/sentry_file_extension.dart 80.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

Comment thread file/analysis_options.yaml
///
/// All the copy, create, delete, open, rename, read, and write operations are
/// supported.
File sentryTrace({

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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,
    );
  }
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah, that sounds reasonable

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I was mostly asking because IOOverrides allow you to also measure the performance, if you don't otherwise control the file io yourself. Like for example with Hive, sembast or similar.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@marandaneto
marandaneto marked this pull request as ready for review December 5, 2022 13:14
@marandaneto

Copy link
Copy Markdown
Contributor Author

@krystofwoldrich @brustolin this is ready for review

Comment thread .craft.yml Outdated
Comment thread .craft.yml Outdated
Comment thread file/lib/src/sentry_file.dart

@brustolin brustolin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM


data = callback() as T;

if (!hasLength) {

@krystofwoldrich krystofwoldrich Dec 6, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@github-actions

github-actions Bot commented Dec 6, 2022

Copy link
Copy Markdown
Contributor

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 293.88 ms 345.11 ms 51.23 ms
Size 5.94 MiB 6.96 MiB 1.02 MiB

Baseline results on branch: main

Startup times

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

Previous results on branch: feat/sentry_file

Startup times

Revision Plain With Sentry Diff
964eb27 297.44 ms 363.69 ms 66.25 ms

App size

Revision Plain With Sentry Diff
964eb27 5.94 MiB 6.96 MiB 1.02 MiB

@github-actions

github-actions Bot commented Dec 6, 2022

Copy link
Copy Markdown
Contributor

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1257.62 ms 1270.47 ms 12.85 ms
Size 8.16 MiB 9.17 MiB 1.01 MiB

Baseline results on branch: main

Startup times

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

Previous results on branch: feat/sentry_file

Startup times

Revision Plain With Sentry Diff
964eb27 1254.98 ms 1262.08 ms 7.10 ms

App size

Revision Plain With Sentry Diff
964eb27 8.16 MiB 9.17 MiB 1.01 MiB

@marandaneto
marandaneto merged commit b47809a into main Dec 6, 2022
@marandaneto
marandaneto deleted the feat/sentry_file branch December 6, 2022 14:18
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.

Official support for File I/O auto performance

6 participants