Skip to content

feat: Scope sync attachments#5038

Merged
Flash0ver merged 15 commits intomainfrom
feat/scope-sync-attachments
Mar 24, 2026
Merged

feat: Scope sync attachments#5038
Flash0ver merged 15 commits intomainfrom
feat/scope-sync-attachments

Conversation

@bitsandfoxes
Copy link
Contributor

@bitsandfoxes bitsandfoxes commented Mar 18, 2026

Relates to getsentry/sentry-native#1584 and getsentry/sentry-java#5211

Allows syncing attachments to native layers. This will require a followup to implement the actual Android/iOS scope sync but unblocks Unity SDK.

#skip-changelog

@github-actions
Copy link
Contributor

github-actions bot commented Mar 18, 2026

Semver Impact of This PR

None (no version bump detected)

📋 Changelog Preview

This is how your changes will appear in the changelog.
Entries from this PR are highlighted with a left border (blockquote style).


This PR will not appear in the changelog.


🤖 This preview updates automatically when you update the PR.

@bitsandfoxes bitsandfoxes marked this pull request as draft March 18, 2026 16:25
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

@bitsandfoxes bitsandfoxes force-pushed the feat/scope-sync-attachments branch from c6cc055 to 2f13317 Compare March 19, 2026 12:24
Co-authored-by: sentry-warden[bot] <258096371+sentry-warden[bot]@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Mar 19, 2026

Codecov Report

❌ Patch coverage is 52.17391% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.02%. Comparing base (248f455) to head (0325400).
⚠️ Report is 40 commits behind head on main.

Files with missing lines Patch % Lines
src/Sentry/Internal/ScopeObserver.cs 0.00% 8 Missing ⚠️
src/Sentry/Platforms/Native/NativeScopeObserver.cs 0.00% 2 Missing ⚠️
src/Sentry/Scope.cs 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5038      +/-   ##
==========================================
+ Coverage   73.94%   74.02%   +0.07%     
==========================================
  Files         497      499       +2     
  Lines       17974    18065      +91     
  Branches     3517     3518       +1     
==========================================
+ Hits        13291    13372      +81     
- Misses       3825     3835      +10     
  Partials      858      858              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

{
other.AddAttachment(attachment);
// Set the attachment directly to avoid triggering a scope sync
other._attachments.Add(attachment);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The pushing and popping the scope does not get propagated to the native layer. So syncing the same attachments again doesn't make sense.

Copy link
Collaborator

@jamescrosswell jamescrosswell Mar 23, 2026

Choose a reason for hiding this comment

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

Android and iOS would both be using Global mode anyway, so no pushing/popping of scopes.

I am wondering what this means for native AOT applications that are using the Native SDK though. For ASP.NET Core apps, for example, we wouldn't want attachments from one async local scope context to end up being synced to some 'global' scope in the native SDK (and an attachment from one request ending up being captured in a native event for a completely unrelated request).

To be clear, I'm not sure if this is possible - just a concern that popped up when you made this remark.

I've added a note to #5052 to investigate this before implementing for Native.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

an attachment from one request ending up being captured in a native event for a completely unrelated request)

A possible solution to this is to limit the attachment sync to global scope mode. That would also be a simple mental model: "global scope persists across all layers".

Copy link
Collaborator

Choose a reason for hiding this comment

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

A possible solution to this is to limit the attachment sync to global scope mode.

That's a good idea... I think probably the only safe way to do it 👍🏻

@bitsandfoxes bitsandfoxes marked this pull request as ready for review March 20, 2026 11:29
Copy link
Collaborator

@jamescrosswell jamescrosswell Mar 23, 2026

Choose a reason for hiding this comment

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

This was actually just added in 0.13.3 of the Native SDK:

Added #5052 as a follow up:

@jamescrosswell
Copy link
Collaborator

@bitsandfoxes this looks fine. The only thing I'm wondering is whether we're likely to implement this for Android, iOS and Native... for native, I can see some potential issues. If we don't end up implementing for one/more of them, I guess we can just add some comments to the relevant Impl methods explaining why. It's a bit weird having abstract members that aren't implemented in some descendants (potentially a base implementation that is a no-op is preferable)... but they're all internal anyway, so we can always refactor once we've investigated the other platforms.

@Flash0ver does that sound good to you?

@Flash0ver
Copy link
Member

Flash0ver commented Mar 24, 2026

@bitsandfoxes this looks fine. The only thing I'm wondering is whether we're likely to implement this for Android, iOS and Native... for native, I can see some potential issues. If we don't end up implementing for one/more of them, I guess we can just add some comments to the relevant Impl methods explaining why. It's a bit weird having abstract members that aren't implemented in some descendants (potentially a base implementation that is a no-op is preferable)... but they're all internal anyway, so we can always refactor once we've investigated the other platforms.

@Flash0ver does that sound good to you?

Yes.
I guess we could instead make the "no-op" implementations virtual rather than abstract.
But currently we are locally consistent which also has some value.

If we later don't actually have an implementation for native,
I believe we still could make the ...Impl methods virtual instead of abstract (to not have an override implementation that's just no-op) without a breaking change ... I think ... the other way around (from virtual to abstract) would be a breaking change, though.

@bitsandfoxes
Copy link
Contributor Author

question: does FilePath need to be accessible for Unity?

Yes, both Byte and FilePath were made internal to be accessible from inside the Unity SDK, hence the tests.

@Flash0ver Flash0ver merged commit 2db04c4 into main Mar 24, 2026
45 checks passed
@Flash0ver Flash0ver deleted the feat/scope-sync-attachments branch March 24, 2026 12:59
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.

3 participants