Feat: Attachment Support

[ueman]

📜 Description

💡 Motivation and Context

Closes #289

💚 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

[codecov-commenter]

Codecov Report

Merging #505 (5727ef1) into main (679dce9) will increase coverage by 0.10%.
The diff coverage is 95.40%.

@@            Coverage Diff             @@
##             main     #505      +/-   ##
==========================================
+ Coverage   91.67%   91.77%   +0.10%     
==========================================
  Files          70       73       +3     
  Lines        2318     2371      +53     
==========================================
+ Hits         2125     2176      +51     
- Misses        193      195       +2     

Impacted Files	Coverage Δ
dart/lib/src/transport/noop_transport.dart	0.00% <0.00%> (ø)
dart/lib/src/sentry_client.dart	94.93% <80.00%> (-1.12%)	⬇️
dart/lib/src/protocol/sentry_id.dart	84.61% <81.81%> (-15.39%)	⬇️
dart/lib/src/scope.dart	94.49% <100.00%> (+1.42%)	⬆️
...ib/src/sentry_attachment/io_sentry_attachment.dart	100.00% <100.00%> (ø)
...t/lib/src/sentry_attachment/sentry_attachment.dart	100.00% <100.00%> (ø)
dart/lib/src/sentry_envelope.dart	100.00% <100.00%> (ø)
dart/lib/src/sentry_envelope_item.dart	100.00% <100.00%> (ø)
dart/lib/src/sentry_envelope_item_header.dart	100.00% <100.00%> (ø)
dart/lib/src/transport/http_transport.dart	100.00% <100.00%> (ø)
... and 6 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 679dce9...5727ef1. Read the comment docs.

[ueman]

This does not yet include offline caching of attachments.

[ueman]

I think there's currently no way to add attachments which need to be awaited. E.g. files and paths. The others are theoretically good, though I expect in practice it's not because they need to be awaited previously.

This is because the scope callback does not accept futures.

[marandaneto]

This does not yet include offline caching of attachments.

what do you mean by that?
once you add an attachment to the scope and call eg captureException(x), the generated envelope will contain the attachment in it so it'd be cached in the disk and sent on restart. (Android/iOS/macOS at least)

[marandaneto]

I think there's currently no way to add attachments which need to be awaited. E.g. files and paths. The others are theoretically good, though I expect in practice it's not because they need to be awaited previously.

This is because the scope callback does not accept futures.

not sure if I follow, what do you mean? I didn't review the PR yet so I miss context

[marandaneto]

nice PR @ueman its a Draft but I already have a great feeling about that, thanks.

[marandaneto]

we will talk about getsentry/sentry-java#1517 this week, depending of the output of the meeting, we can address this issue in this PR too

[ueman]

I think there's currently no way to add attachments which need to be awaited. E.g. files and paths. The others are theoretically good, though I expect in practice it's not because they need to be awaited previously.
This is because the scope callback does not accept futures.

not sure if I follow, what do you mean? I didn't review the PR yet so I miss context

You'll have to use async/await

Sentry.configureScope((scope) async {
    scope.addAttachment(await Attachment.fromFile(file));
}):

in order to add an attachment from a file. Though the callback doesn't allow it IIRC.

You have to do this instead:

final attachment = await Attachment.fromFile(file);
Sentry.configureScope((scope) {
    scope.addAttachment(attachment);
}):

Same thing for Sentry.captureEvent(event, withScope: (scope){...})

I mean you can use it without, so it's not a blocker.

[marandaneto]

spec https://develop.sentry.dev/sdk/features/#attachments
we dont have to consider addToTransactions right now

[marandaneto]

I think there's currently no way to add attachments which need to be awaited. E.g. files and paths. The others are theoretically good, though I expect in practice it's not because they need to be awaited previously.
This is because the scope callback does not accept futures.

not sure if I follow, what do you mean? I didn't review the PR yet so I miss context

You'll have to use async/await

Sentry.configureScope((scope) async {
    scope.addAttachment(await Attachment.fromFile(file));
}):

in order to add an attachment from a file. Though the callback doesn't allow it IIRC.

You have to do this instead:

final attachment = await Attachment.fromFile(file);
Sentry.configureScope((scope) {
    scope.addAttachment(attachment);
}):

Same thing for Sentry.captureEvent(event, withScope: (scope){...})

I mean you can use it without, so it's not a blocker.

See https://develop.sentry.dev/sdk/features/#attachments

The SDK should read the file when an event gets captured and not when the user adds an attachment to the scope.

[bruno-garcia]

Disclaimer: I didn't read/review the PR. Just wanted to throw in some challenges we learned since releasing the attachment API in some SDKs so far:

• How can we attach a file when an event is captured through the uncaught exception handler?
• Is it possible to remove an attachment in a callback of sorts?
• How easy will it be once we add a feature such as attachScreenshot (could be an extension method) that will result in a screenshot taken at the time the event is being created (and not at the time the method is called). See this workaround.
• Does the API expose a way to add an attachment 'only once'?

Ideas that could solve these cases ^ would be welcomed

[ueman]

• How can we attach a file when an event is captured through the uncaught exception handler?

Flutter apps don't quit on uncaught exceptions in Dart, so this is a non issue. If an exception happens in the native part the Dart uncaught exception handler is never called at all I guess. We would need to sync the scope across SDKs for that.

• Is it possible to remove an attachment in a callback of sorts?

No, though @marandaneto already raised this issue, too.

• How easy will it be once we add a feature such as attachScreenshot (could be an extension method) that will result in a screenshot taken at the time the event is being created (and not at the time the method is called). See this workaround.

This is quite an interesting problem.
Taking a screenshot is easy (given we hooked into the UI), because Flutter does not have app quitting exceptions, so it should always be meaningful.
However there is no automatic way to hook into the Flutter UI to take the screenshot at all. Also screenshots don't work on Flutter Web with the HTML renderer
and Platform Views are invisible on all platforms.

It probably makes sense to use the native part, if the time it takes to call the native platform has a low enough latency.

• Does the API expose a way to add an attachment 'only once'?

I think you can use Sentry.captureEvent(event, withScope: (scope){...}) for that if I understand the question correctly.

Ideas that could solve these cases ^ would be welcomed

[marandaneto]

@ueman

If an exception happens in the native part the Dart uncaught exception handler is never called at all I guess. We would need to sync the scope across SDKs for that.

that's right, could be worked on #194

This is quite an interesting problem.
Taking a screenshot is easy (given we hooked into the UI), because Flutter does not have app quitting exceptions, so it should always be meaningful.
However there is no automatic way to hook into the Flutter UI to take the screenshot at all. Also screenshots don't work on Flutter Web with the HTML renderer
and Platform Views are invisible on all platforms.

indeed, the workaround in the symbol-collector right now would not be possible as configureScope isn't async so you cant read the stream and await for it.
about Flutter Web, we can make this available only for Flutter io, its fine.
the idea is that if the options is enabled and they hooked up our integration manually when initing the SDK, we take a screenshot when runZonedGuarded is called, runZonedGuarded allows async code so I think its fine to take the screenshot and read the stream, add an attachment and capture it.
We just need to be sure that the zone in runZonedGuarded allows taking the screenshot or not (because of main/UI thread)

I think you can use Sentry.captureEvent(event, withScope: (scope){...}) for that if I understand the question correctly.

it'd be a workaround but that's not quite the same thing.
the idea is that addAttachment gets an optional param to be sent only once and the event can be captured later at some point and the attachment delete itself from the attachment list.
with the withScope you pretty much need to capture the event right away which is not the case most of the times.

[philipphofmann]

Didn't finish yet, will do another pass tomorrow.

[marandaneto]

consider adding a method called clearAttachments to the Scope, similar to getsentry/sentry-dotnet#1088

[philipphofmann]

consider adding a method called clearAttachments to the Scope, similar to getsentry/sentry-dotnet#1088

Just wanted to add a comment for this. Thanks @marandaneto 🚀

[philipphofmann]

Just found one more API issue.

[ueman]

@philipphofmann Just to be sure:
h is for high, m for medium and l stands for low, right?

[marandaneto]

@ueman is it ready for final code review? @bruno-garcia or @philipphofmann will do it, thanks

[ueman]

@ueman is it ready for final code review? @bruno-garcia or @philipphofmann will do it, thanks

Yep, it's ready now.

[marandaneto]

LGTM