Tracing for File IO integration #1160

marandaneto · 2022-12-02T10:00:51Z

📜 Description

Tracing for File IO integration

💡 Motivation and Context

Closes #835

💚 How did you test it?

📝 Checklist

I reviewed submitted code
I added tests to verify changes
I updated the docs if needed -> Add File IO instrumentation docs sentry-docs#5895
All tests passing
No breaking changes

🔮 Next steps

marandaneto · 2022-12-02T10:01:25Z

.craft.yml

  - name: github
  - name: registry
    sdks:
      pub:sentry:
      pub:sentry_flutter:
      pub:sentry_logging:
      pub:sentry_dio:
+      #pub:sentry_dio:


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

@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

Yes, it's still "broken"

github-actions · 2022-12-02T10:02:30Z

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

Generated by 🚫 dangerJS against de0ca3c

codecov-commenter · 2022-12-02T10:03:54Z

Codecov Report

Base: 90.14% // Head: 76.45% // Decreases project coverage by -13.68% ⚠️

Coverage data is based on head (de0ca3c) compared to base (62dde43).
Patch coverage: 55.07% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1160       +/-   ##
===========================================
- Coverage   90.14%   76.45%   -13.69%     
===========================================
  Files         119       11      -108     
  Lines        3672      327     -3345     
===========================================
- Hits         3310      250     -3060     
+ Misses        362       77      -285

Impacted Files	Coverage Δ
file/lib/src/sentry_file.dart	`54.13% <54.13%> (ø)`
file/lib/src/sentry_file_extension.dart	`80.00% <80.00%> (ø)`
dart/lib/src/utils/hash_code.dart
dart/lib/src/sentry_client.dart
dart/lib/src/protocol/sentry_culture.dart
dart/lib/src/protocol/debug_image.dart
...rt/lib/src/sentry_client_attachment_processor.dart
dart/lib/src/protocol/sentry_transaction_info.dart
dart/lib/src/http_client/breadcrumb_client.dart
dart/lib/src/transport/rate_limiter.dart
... and 102 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

file/analysis_options.yaml

ueman · 2022-12-05T12:30:45Z

file/lib/src/sentry_file_extension.dart

+  ///
+  /// All the copy, create, delete, open, rename, read, and write operations are
+  /// supported.
+  File sentryTrace({


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

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?

Yeah, that sounds reasonable

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.

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 · 2022-12-05T13:20:47Z

@krystofwoldrich @brustolin this is ready for review

.craft.yml

file/lib/src/sentry_file.dart

brustolin

LGTM

…nto feat/sentry_file

krystofwoldrich · 2022-12-06T09:52:46Z

file/lib/src/sentry_file.dart

+
+      data = callback() as T;
+
+      if (!hasLength) {


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.

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 · 2022-12-06T11:09:30Z

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 · 2022-12-06T11:21:00Z

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

Tracing for File IO integration

85df9db

marandaneto commented Dec 2, 2022

View reviewed changes

marandaneto added 13 commits December 2, 2022 11:19

add file workflow

0e48d00

fix pr id

0f74b5b

copy tests

2a999cd

add tests for create

2f372ac

fix

d6a5de6

fixes

ac4d4dc

add missing tests

05ea2c9

remove

153813b

add extension

1312453

fixes

244a6f4

add example

00cd84a

add readme

5ce347d

fixes comments

74a42f1

ueman reviewed Dec 5, 2022

View reviewed changes

marandaneto added 2 commits December 5, 2022 14:02

enable strict analyzer

52bf20d

add integration

e39ecc7

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

marandaneto requested review from brustolin and krystofwoldrich as code owners December 5, 2022 13:14

add @experimental annotation

1841669

overwrite pana threshold

a794e14

marandaneto mentioned this pull request Dec 5, 2022

Add File IO instrumentation docs getsentry/sentry-docs#5895

Merged

krystofwoldrich reviewed Dec 5, 2022

View reviewed changes

.craft.yml Outdated Show resolved Hide resolved

marandaneto commented Dec 5, 2022

View reviewed changes

.craft.yml Outdated Show resolved Hide resolved

Update .craft.yml

6fe9687

krystofwoldrich reviewed Dec 5, 2022

View reviewed changes

file/lib/src/sentry_file.dart Show resolved Hide resolved

brustolin approved these changes Dec 6, 2022

View reviewed changes

marandaneto added 2 commits December 6, 2022 09:45

remove not needed items from gh action

7b46c5c

Merge branch 'feat/sentry_file' of github.com:getsentry/sentry-dart i…

35215e2

…nto feat/sentry_file

krystofwoldrich reviewed Dec 6, 2022

View reviewed changes

krystofwoldrich approved these changes Dec 6, 2022

View reviewed changes

marandaneto added 2 commits December 6, 2022 11:20

add dart file to min test

88b6194

add example

964eb27

remove example

de0ca3c

marandaneto merged commit b47809a into main Dec 6, 2022

marandaneto deleted the feat/sentry_file branch December 6, 2022 14:18

marandaneto mentioned this pull request Dec 12, 2022

New File interface affects SentryFile #1179

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tracing for File IO integration #1160

Tracing for File IO integration #1160

marandaneto commented Dec 2, 2022 •

edited

marandaneto Dec 2, 2022

marandaneto Dec 5, 2022

kamilogorek Dec 5, 2022

github-actions bot commented Dec 2, 2022 •

edited

codecov-commenter commented Dec 2, 2022 •

edited

ueman Dec 5, 2022

marandaneto Dec 5, 2022

ueman Dec 5, 2022

ueman Dec 5, 2022

marandaneto Dec 5, 2022

marandaneto commented Dec 5, 2022

brustolin left a comment

krystofwoldrich Dec 6, 2022 •

edited

marandaneto Dec 6, 2022

github-actions bot commented Dec 6, 2022 •

edited

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/sentry_file

Startup times

App size

github-actions bot commented Dec 6, 2022 •

edited

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/sentry_file

Startup times

App size

Tracing for File IO integration #1160

Tracing for File IO integration #1160

Conversation

marandaneto commented Dec 2, 2022 • edited

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 2, 2022 • edited

codecov-commenter commented Dec 2, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marandaneto commented Dec 5, 2022

brustolin left a comment

Choose a reason for hiding this comment

krystofwoldrich Dec 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 6, 2022 • edited

Android Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/sentry_file

Startup times

App size

github-actions bot commented Dec 6, 2022 • edited

iOS Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/sentry_file

Startup times

App size

marandaneto commented Dec 2, 2022 •

edited

github-actions bot commented Dec 2, 2022 •

edited

codecov-commenter commented Dec 2, 2022 •

edited

krystofwoldrich Dec 6, 2022 •

edited

github-actions bot commented Dec 6, 2022 •

edited

github-actions bot commented Dec 6, 2022 •

edited