Add runtime attachments support for Windows/Linux

[tustanivsky]

This PR adopts changes introduced in Native SDK version 0.9.1 to enable runtime attachments on Windows and Linux.

Attachments can now be added to all outgoing events by setting them via USentrySubsystem on the global scope or to individual events by adding them to a local scope, i.e. when capturing with CaptureEventWithScope.

On Apple/Android, removing individual attachments from the global scope currently is not supported. However, on these platforms the corresponding SDKs provide clearAttachments which is not available in sentry-native at the moment.

Closes #430
Closes #946

[github-actions]

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

Generated by 🚫 dangerJS against 19da3b2

[jpnurmi]

There might be some unintentional copies when passing the byte buffer around:

const TArray<uint8>& byteBuf = attachment->GetData();
...

The reference makes it look like no copy is happening but GetData() returns TArray<uint8> by value. Since it returns a member variable and not a temporary, RVO won't kick in either. If it's ok for the internal platform interface to return by reference, that would save us some copies.

[tustanivsky]

There might be some unintentional copies when passing the byte buffer around:

const TArray<uint8>& byteBuf = attachment->GetData();
...

The reference makes it look like no copy is happening but GetData() returns TArray<uint8> by value. Since it returns a member variable and not a temporary, RVO won't kick in either. If it's ok for the internal platform interface to return by reference, that would save us some copies.

@jpnurmi On Android and iOS we're retrieving attachment's byte data from the corresponding native objects via conversion utils so we can't really return it by reference as result is a temporary.

However, we can introduce a GetDataByRef() method specifically for FGenericPlatformSentryAttachment without changing the platform interface. This would avoid unnecessary copying of byte arrays when adding them to the scope. If the user calls GetData() a copy will still be made but in that case the API makes it explicit. Does that make sense?

[jpnurmi]

However, we can introduce a GetDataByRef() method specifically for FGenericPlatformSentryAttachment without changing the platform interface. This would avoid unnecessary copying of byte arrays when adding them to the scope. If the user calls GetData() a copy will still be made but in that case the API makes it explicit. Does that make sense?

Makes sense! 👍 I'm not sure how often people use larger buffers and whether temporary copies even matter here, but sentry-native also makes its own copy a few lines later, so it made me wonder if we could somehow avoid the unnecessary temporary extra copies... :)

[limbonaut]

This may become a gotcha moment for users. Maybe this should be raised internally. Is it necessary to have the removeAttachment method on a global scope? If so, should it also be included in the underlying SDKs?
Same issue in Godot SDK: getsentry/sentry-godot#205 (comment)

[tustanivsky]

Given the discrepancies between Native, Android, and Cocoa in terms of attachment API support it looks like we have the following options:

1. Add clearAttachments method to Native which is already available for Android/Cocoa and expose that for global scope instead of giving user the ability to remove individual attachments
2. Add removeAttachment to Android/Cocoa to enable removing individual attachments on mobile
3. Combine 1 and 2 to cover all possible use cases
4. Leave only addAttachment for global scope without the ability to remove it
5. Leave things as-is and document the current limitations

[jpnurmi]

I don't see any problems with adding a new sentry-native method to clear attachments in the global scope.

[tustanivsky]

Sounds good - we can expose only the AddAttachment and ClearAttachments methods for the global scope to align with the mobile SDKs.

This could also simplify the FGenericPlatformSentryAttachment implementation as we’d no longer need to store a sentry_attachment_t reference there.

[limbonaut]

Wouldn't clear_attachments() also nuke a screenshot attachment? I wonder if it's going to be an unfortunate side-effect.

[jpnurmi]

The problem kind of bubbles up. Crashpad doesn't know which attachments are special from sentry-native's perspective. Likewise, sentry-native might not know which attachments are special from a downstream SDKs perspective.

I like the idea of making a distinction between fixed attachments added via options vs. dynamic attachments added via the new API, even though during sentry-native attachment API reviews it was proposed that the old options API could be removed when the missing Crashpad IPC was implemented for macOS. Not sure if it's going to happen anytime soon for macOS using rather complicated Mach ports for IPC, though. 😅

[tustanivsky]

For now, I’ve worked around this by implementing ClearAttachments for Win/Linux on the Unreal side of things (see commit).

The idea is to store user-defined attachments in an internal vector and remove them one by one using the native sentry_remove_attachment whenever ClearAttachments is called. This way, we avoid accidentally removing any attachments that were added during initialization.

@jpnurmi wdyt?

[jpnurmi]

Sounds ok. In principle, sentry_clear_attachments() works the same way with Crashpad to avoid removing the internal __sentry-event and __sentry-breadcrumbN attachments.

[jpnurmi]

Btw, not saying we can't or shouldn't improve this later on! 🙂 But I think this is fine for now.

[tustanivsky]

Alright, I believe we're no longer blocked here and can proceed with the merge. Further improvements related to handling attachments may come in subsequent PRs.