-
-
Notifications
You must be signed in to change notification settings - Fork 230
feat: Scope sync attachments #5038
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.
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
Changes from all commits
e0b7d26
53fc737
3735548
2f13317
c6a86de
9d8e1fc
e3bb35b
62c6087
6080ec5
7675409
d623b57
43bcbf0
aa0b353
bab6d51
0325400
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -390,7 +390,14 @@ public void UnsetTag(string key) | |
| /// <summary> | ||
| /// Adds an attachment. | ||
| /// </summary> | ||
| public void AddAttachment(SentryAttachment attachment) => _attachments.Add(attachment); | ||
| public void AddAttachment(SentryAttachment attachment) | ||
| { | ||
| _attachments.Add(attachment); | ||
| if (Options.EnableScopeSync) | ||
| { | ||
| Options.ScopeObserver?.AddAttachment(attachment); | ||
| } | ||
| } | ||
|
|
||
| internal void SetPropagationContext(SentryPropagationContext propagationContext) | ||
| { | ||
|
|
@@ -433,6 +440,10 @@ public void ClearAttachments() | |
| #else | ||
| _attachments.Clear(); | ||
| #endif | ||
| if (Options.EnableScopeSync) | ||
| { | ||
| Options.ScopeObserver?.ClearAttachments(); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -535,7 +546,8 @@ public void Apply(Scope other) | |
|
|
||
| foreach (var attachment in Attachments) | ||
| { | ||
| other.AddAttachment(attachment); | ||
| // Set the attachment directly to avoid triggering a scope sync | ||
| other._attachments.Add(attachment); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 would also be a simple mental model: "global scope persists across all layers".
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's a good idea... I think probably the only safe way to do it 👍🏻 |
||
| } | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.