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.

Sign up for GitHub

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

Jump to bottom

Improve FileSystemTransport by using compute() #1811

Closed
wants to merge 6 commits into from
Closed

Improve FileSystemTransport by using compute() #1811

wants to merge 6 commits into from

Conversation

denrase
Copy link
Collaborator

@denrase denrase commented Jan 9, 2024 •
edited
Loading

📜 Description

  • Use compute to convert the SentryEnvelope to binary data.
  • Changed the method signature of envelopeStream to just take int maxAttachmentSize instead of SentryOptions object.

💡 Motivation and Context

Closes #536

💚 How did you test it?

  • Run example app, as there are no tests AFAIK that guarantee correct envelope data transfer to the mobile plugins.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

  • We should think about profiling if this has a positive impact. Cant really measure, as the runtime would be the same, it would just be on a background isolate.
  • We are using closures to delay envelope item to binary conversion. Can we guarantee we won't close over objects that are not supported to be sent to another Isolate? -> NO
SentryTransaction
- SentrySpan
 - SentrySpanContext
   - Hub
     - SentryTracesSampler
       - SentryOptions
     - SentryOptions

Through the closures we have a reference to SentryOptions, and therefore potentially always to objects which we cannot send. See examples below, tested on the example app.

Capturing a sentry transaction.

[sentry] [error] Error while capturing transaction with id: b25abc26513d4d6c95e9ca2bd853395f
         Invalid argument(s): Illegal argument in isolate message: (object is a Pointer)
 <- __objc_msgSend_1058Ptr in Instance of 'Sentr...
         #0      Isolate._spawnFunction (dart:isolate-patch/isolate_patch.dart:398:25)
         #1      Isolate.spawn (dart:isolate-patch/isolate_patch.dart:378:7)
         #2      Isolate.run (dart:isolate:285:15)
         #3      compute (package:flutter/src/foundation/_isolates_io.dart:18:18)
         #4      compute (package:flutter/src/foundation/isolates.dart:76:19)
         #5      FileSystemTransport.send (package:sentry_flutter/src/file_system_transport.dart:17:32)
         #6      SentryClient._attachClientReportsAndSend (package:sentry/src/sentry_client.dart:517:31)
         #7      SentryClient.captureEnvelope (package:sentry/src/sentry_client.dart:364:12)
         #8      SentryClient.captureTransaction (package:sentry/src/sentry_client.dart:357:22)
         <asynchronous suspension>
         #9      Hub.captureTransaction (package:sentry/src/hub.dart:539:22)
         <asynchronous suspension>
         #10     SentryTracer.finish (package:sentry/src/sentry_tracer.dart:153:7)
         <asynchronous suspension>
         #11     makeWebRequest (package:sentry_flutter_example/main.dart:909:3)
         <asynchronous suspension>

Passing SentryOptions to the compute function. For example, the IsolateErrorIntegration has a reference to RawReceivePort.

         Invalid argument(s): Illegal argument in isolate message: (object is a ReceivePort)
 <- _receivePort in Instance of 'IsolateErro...
         #0      Isolate._spawnFunction (dart:isolate-patch/isolate_patch.dart:398:25)
         #1      Isolate.spawn (dart:isolate-patch/isolate_patch.dart:378:7)
         #2      Isolate.run (dart:isolate:285:15)
         #3      compute (package:flutter/src/foundation/_isolates_io.dart:18:18)
         #4      compute (package:flutter/src/foundation/isolates.dart:76:19)
         #5      FileSystemTransport.send (package:sentry_flutter/src/file_system_transport.dart:17:32)
         #6      SentryClient._attachClientReportsAndSend (package:sentry/src/sentry_client.dart:517:31)
         #7      SentryClient.captureEnvelope (package:sentry/src/sentry_client.dart:364:12)
         #8      SentryClient.captureEvent (package:sentry/src/sentry_client.dart:151:22)
         <asynchronous suspension>
         #9      Hub.captureException (package:sentry/src/hub.dart:161:20)
         <asynchronous suspension>
         #10     tryCatch (package:sentry_flutter_example/main.dart:738:5)
         <asynchronous suspension>

When commenting it out, we still fail as we have captured a Pointer somewhere.

         Invalid argument(s): Illegal argument in isolate message: (object is a Pointer)
 <- __objc_msgSend_1058Ptr in Instance of 'Sentr...
         #0      Isolate._spawnFunction (dart:isolate-patch/isolate_patch.dart:398:25)
         #1      Isolate.spawn (dart:isolate-patch/isolate_patch.dart:378:7)
         #2      Isolate.run (dart:isolate:285:15)
         #3      compute (package:flutter/src/foundation/_isolates_io.dart:18:18)
         #4      compute (package:flutter/src/foundation/isolates.dart:76:19)
         #5      FileSystemTransport.send (package:sentry_flutter/src/file_system_transport.dart:17:32)
         #6      SentryClient._attachClientReportsAndSend (package:sentry/src/sentry_client.dart:517:31)
         #7      SentryClient.captureEnvelope (package:sentry/src/sentry_client.dart:364:12)
         #8      SentryClient.captureEvent (package:sentry/src/sentry_client.dart:151:22)
         <asynchronous suspension>
         #9      Hub.captureException (package:sentry/src/hub.dart:161:20)
         <asynchronous suspension>
         #10     tryCatch (package:sentry_flutter_example/main.dart:738:5)
         <asynchronous suspension>

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 9, 2024 •
edited
Loading

Fails
🚫 Please consider adding a changelog entry for the next release.

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

- Improve `FileSystemTransport` by using `compute()` ([#1811](https://github.com/getsentry/sentry-dart/pull/1811))

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 6b4e51a

@codecov Codecov
Copy link

codecov bot commented Jan 9, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (552c543) 88.55% compared to head (6b4e51a) 88.52%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1811      +/-   ##
==========================================
- Coverage   88.55%   88.52%   -0.04%     
==========================================
  Files         206      206              
  Lines        6870     6874       +4     
==========================================
+ Hits         6084     6085       +1     
- Misses        786      789       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 9, 2024 •
edited
Loading

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 380.10 ms 458.65 ms 78.55 ms
Size 6.33 MiB 7.26 MiB 951.08 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
f79eecf 341.35 ms 388.98 ms 47.63 ms
64af39c 386.80 ms 471.11 ms 84.31 ms
8609bd8 359.76 ms 437.40 ms 77.64 ms
1e781fc 315.51 ms 383.76 ms 68.24 ms
322aa66 284.98 ms 341.76 ms 56.78 ms
f96ca24 316.08 ms 388.22 ms 72.14 ms
9f9f94f 331.04 ms 368.92 ms 37.88 ms
8932ece 309.56 ms 377.28 ms 67.72 ms
0a82a1e 321.02 ms 393.82 ms 72.80 ms
22ed6cb 326.27 ms 393.00 ms 66.73 ms

App size

Revision Plain With Sentry Diff
f79eecf 6.15 MiB 7.13 MiB 1000.07 KiB
64af39c 6.27 MiB 7.20 MiB 958.83 KiB
8609bd8 6.27 MiB 7.20 MiB 959.07 KiB
1e781fc 6.06 MiB 7.03 MiB 995.72 KiB
322aa66 5.94 MiB 6.92 MiB 1005.75 KiB
f96ca24 6.06 MiB 7.10 MiB 1.04 MiB
9f9f94f 5.94 MiB 6.95 MiB 1.01 MiB
8932ece 6.16 MiB 7.13 MiB 1000.89 KiB
0a82a1e 6.15 MiB 7.11 MiB 981.82 KiB
22ed6cb 6.06 MiB 7.03 MiB 993.37 KiB

Previous results on branch: enha/compute-transport

Startup times

Revision Plain With Sentry Diff
7cdb71f 376.20 ms 451.33 ms 75.12 ms
dfb982b 362.36 ms 446.94 ms 84.58 ms
6990b1a 370.75 ms 429.73 ms 58.98 ms

App size

Revision Plain With Sentry Diff
7cdb71f 6.33 MiB 7.26 MiB 949.06 KiB
dfb982b 6.33 MiB 7.26 MiB 949.03 KiB
6990b1a 6.33 MiB 7.26 MiB 949.88 KiB

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 9, 2024 •
edited
Loading

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1248.90 ms 1266.16 ms 17.27 ms
Size 8.32 MiB 9.39 MiB 1.06 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
cd16818 1254.78 ms 1267.76 ms 12.98 ms
5e8d2b3 1255.71 ms 1267.98 ms 12.27 ms
b47809a 1252.61 ms 1278.57 ms 25.96 ms
f0fcbe1 1238.08 ms 1243.44 ms 5.36 ms
a61674e 1275.51 ms 1290.81 ms 15.30 ms
ca7f531 1242.51 ms 1282.61 ms 40.10 ms
ccc09e4 1254.74 ms 1277.08 ms 22.34 ms
62dde43 1258.43 ms 1276.81 ms 18.38 ms
26e955b 1232.35 ms 1258.88 ms 26.52 ms
6e083bb 1244.33 ms 1264.60 ms 20.26 ms

App size

Revision Plain With Sentry Diff
cd16818 8.28 MiB 9.33 MiB 1.05 MiB
5e8d2b3 8.29 MiB 9.36 MiB 1.07 MiB
b47809a 8.16 MiB 9.17 MiB 1.01 MiB
f0fcbe1 8.29 MiB 9.38 MiB 1.09 MiB
a61674e 8.10 MiB 9.16 MiB 1.07 MiB
ca7f531 8.32 MiB 9.38 MiB 1.06 MiB
ccc09e4 8.16 MiB 9.16 MiB 1.01 MiB
62dde43 8.16 MiB 9.17 MiB 1.01 MiB
26e955b 8.28 MiB 9.34 MiB 1.05 MiB
6e083bb 8.16 MiB 9.17 MiB 1.01 MiB

Previous results on branch: enha/compute-transport

Startup times

Revision Plain With Sentry Diff
dfb982b 1206.96 ms 1235.57 ms 28.61 ms
7cdb71f 1206.71 ms 1226.16 ms 19.45 ms
6990b1a 1240.53 ms 1258.96 ms 18.43 ms

App size

Revision Plain With Sentry Diff
dfb982b 8.32 MiB 9.39 MiB 1.06 MiB
7cdb71f 8.32 MiB 9.39 MiB 1.06 MiB
6990b1a 8.32 MiB 9.39 MiB 1.06 MiB

@denrase
Copy link
Collaborator Author

denrase commented Jan 22, 2024

@buenaflor I thin we cannot offload the work to another Isolate, as we are basically capturing the whole SDK through the closures used to compute the data for envelope items.

It seems we also can't use channels im background, as the API is only available for flutter 3.7, and we'd need to call a static field not present on lower flutter versions.

@denrase denrase mentioned this pull request Jan 30, 2024
Open
@denrase denrase closed this Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krystofwoldrich krystofwoldrich Awaiting requested review from krystofwoldrich krystofwoldrich will be requested when the pull request is marked ready for review krystofwoldrich is a code owner

@stefanosiano stefanosiano Awaiting requested review from stefanosiano stefanosiano will be requested when the pull request is marked ready for review stefanosiano is a code owner

@buenaflor buenaflor Awaiting requested review from buenaflor buenaflor will be requested when the pull request is marked ready for review buenaflor is a code owner

@martinhaintz martinhaintz Awaiting requested review from martinhaintz martinhaintz will be requested when the pull request is marked ready for review martinhaintz is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve FileSystemTransport by using compute() or background isolates
1 participant
@denrase