-
-
Notifications
You must be signed in to change notification settings - Fork 372
feat: Add attributes data to SentryScope
#6830
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
base: main
Are you sure you want to change the base?
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6830 +/- ##
========================================
Coverage ? 84.671%
========================================
Files ? 452
Lines ? 27616
Branches ? 12103
========================================
Hits ? 23383
Misses ? 4187
Partials ? 46
Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1339919 | 1214.82 ms | 1238.98 ms | 24.16 ms |
| 319fb1e | 1219.48 ms | 1242.69 ms | 23.21 ms |
| 3ec47ae | 1231.02 ms | 1256.67 ms | 25.65 ms |
| e58d7bf | 1219.98 ms | 1242.39 ms | 22.41 ms |
| 85f7349 | 1232.28 ms | 1248.27 ms | 15.98 ms |
| 7f26f16 | 1220.62 ms | 1255.04 ms | 34.42 ms |
| 119ab1c | 1226.79 ms | 1254.55 ms | 27.76 ms |
| 1fe932f | 1231.92 ms | 1253.44 ms | 21.52 ms |
| 139db8b | 1231.50 ms | 1258.19 ms | 26.69 ms |
| a4c5ddc | 1239.61 ms | 1266.41 ms | 26.80 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1339919 | 23.75 KiB | 919.70 KiB | 895.95 KiB |
| 319fb1e | 23.75 KiB | 1019.18 KiB | 995.43 KiB |
| 3ec47ae | 23.75 KiB | 919.88 KiB | 896.13 KiB |
| e58d7bf | 24.15 KiB | 1.01 MiB | 1014.91 KiB |
| 85f7349 | 23.75 KiB | 1.01 MiB | 1006.48 KiB |
| 7f26f16 | 23.75 KiB | 1.02 MiB | 1016.66 KiB |
| 119ab1c | 23.75 KiB | 993.70 KiB | 969.95 KiB |
| 1fe932f | 23.75 KiB | 913.63 KiB | 889.88 KiB |
| 139db8b | 23.75 KiB | 920.64 KiB | 896.89 KiB |
| a4c5ddc | 23.75 KiB | 977.30 KiB | 953.55 KiB |
| self.contextDictionary = [NSMutableDictionary new]; | ||
| self.attachmentArray = [NSMutableArray new]; | ||
| self.fingerprintArray = [NSMutableArray new]; | ||
| self.attributesDictionary = [NSMutableDictionary new]; | ||
| _spanLock = [[NSObject alloc] init]; | ||
| self.observers = [NSMutableArray new]; | ||
| self.propagationContext = [[SentryPropagationContext alloc] init]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: initWithScope: does not copy _attributesDictionary, leading to silent data loss during scope duplication.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The initWithScope: method fails to copy the _attributesDictionary from the source scope. This omission means that when a scope is duplicated using initWithScope:, any attributes set on the original scope are silently lost, breaking the expected copy semantics. This is critical as attributes are a new feature intended to behave like other scope data types.
💡 Suggested Fix
In initWithScope:, add [_attributesDictionary addEntriesFromDictionary:[scope attributes]]; to ensure _attributesDictionary is copied.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: Sources/Sentry/SentryScope.m#L51-L57
Potential issue: The `initWithScope:` method fails to copy the `_attributesDictionary`
from the source scope. This omission means that when a scope is duplicated using
`initWithScope:`, any attributes set on the original scope are silently lost, breaking
the expected copy semantics. This is critical as `attributes` are a new feature intended
to behave like other scope data types.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2841132
Sources/Sentry/SentryScope.m
Outdated
| [_attributesDictionary removeObjectForKey:key]; | ||
|
|
||
| // ScopeObservers are not called since at this moment attributes are only used for Logs, | ||
| // which the LogBatcher obtains manually. At this moment not even Spans use this attributes. | ||
| // Should this change, we will need to call the observers. | ||
| } | ||
| } | ||
|
|
||
| @end | ||
|
|
||
| NS_ASSUME_NONNULL_END |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: clear() method fails to clear _attributesDictionary, causing attributes to persist after scope clear.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The clear() method does not clear the _attributesDictionary, causing attributes to persist when scope.clear() is called. This results in data leakage, as old attributes from previous scope states remain active, violating the expected semantics of completely resetting all scope data.
💡 Suggested Fix
Add [_attributesDictionary removeAllObjects]; within a @synchronized(_attributesDictionary) block to the clear() method in SentryScope.m.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: Sources/Sentry/SentryScope.m#L675-L714
Potential issue: The `clear()` method does not clear the `_attributesDictionary`,
causing attributes to persist when `scope.clear()` is called. This results in data
leakage, as old attributes from previous scope states remain active, violating the
expected semantics of completely resetting all scope data.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2841132
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
h: Yes, I think that we must remove the attributes also when calling scope.clear. I opened a develop docs PR to clarify that getsentry/sentry-docs#15578, but I'm not 100% it's expected behaviour. Let's wait for Michi to confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Attributes not cleared in clear method
The clear method was not updated to reset the attributesDictionary. As a result, scope.attributes persist on the scope object even after scope.clear() is explicitly called.
Sources/Sentry/SentryScope.m#L170-L203
sentry-cocoa/Sources/Sentry/SentryScope.m
Lines 170 to 203 in 6566e2d
| return nil; | |
| } | |
| - (void)clear | |
| { | |
| // As we need to synchronize the accesses of the arrays and dictionaries and we use the | |
| // references instead of self we remove all objects instead of creating new instances. Removing | |
| // all objects is usually O(n). This is acceptable as we don't expect a huge amount of elements | |
| // in the arrays or dictionaries, that would slow down the performance. | |
| [self clearBreadcrumbs]; | |
| @synchronized(_tagDictionary) { | |
| [_tagDictionary removeAllObjects]; | |
| } | |
| @synchronized(_extraDictionary) { | |
| [_extraDictionary removeAllObjects]; | |
| } | |
| @synchronized(_contextDictionary) { | |
| [_contextDictionary removeAllObjects]; | |
| } | |
| @synchronized(_fingerprintArray) { | |
| [_fingerprintArray removeAllObjects]; | |
| } | |
| [self clearAttachments]; | |
| @synchronized(_spanLock) { | |
| _span = nil; | |
| } | |
| self.userObject = nil; | |
| self.distString = nil; | |
| self.environmentString = nil; | |
| self.levelEnum = kSentryLevelNone; | |
| for (id<SentryScopeObserver> observer in self.observers) { | |
| [observer clear]; |
|
|
||
| @property (atomic, strong) NSMutableArray<SentryBreadcrumb *> *breadcrumbArray; | ||
|
|
||
| @property (atomic, strong) NSMutableDictionary<NSString *, id> *attributesDictionary; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sources/Sentry/SentryScope.m
Outdated
| // which the LogBatcher obtains manually. At this moment not even Spans use this attributes. | ||
| // Should this change, we will need to call the observers. | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also update the SentryScope+Equality.m test file.
Currently units are not supported since Logs don't support them (yet).
Yes, but please double-check that we can add these without a breaking change. I think we should be good because we will simply add method overloads, but it is still worth checking if that works.
We should merge this PR including #6834 to main cause otherwise the attributes are useless. Thanks for splitting this up into two PRs.
| import Sentry | ||
| import UIKit | ||
|
|
||
| class ScopeViewController: UIViewController { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m: What drove you to add a view controller for this? Do we need UI to test this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for manual testing, no UI test were created.
Tests/SentryTests/SentryScopeTests.m
Outdated
| - (void)testSetStringAttribute | ||
| { | ||
| SentryScope *scope = [[SentryScope alloc] init]; | ||
| [scope setAttributeValue:@"test-string" forKey:@"a-string-key"]; | ||
| XCTAssertEqualObjects([scope attributes][@"a-string-key"], @"test-string"); | ||
| } | ||
|
|
||
| - (void)testSetBoolAttribute | ||
| { | ||
| SentryScope *scope = [[SentryScope alloc] init]; | ||
| [scope setAttributeValue:@(true) forKey:@"a-bool-key"]; | ||
| [scope setAttributeValue:@(false) forKey:@"a-bool-key-false"]; | ||
| XCTAssertEqualObjects([scope attributes][@"a-bool-key"], @(true)); | ||
| XCTAssertEqualObjects([scope attributes][@"a-bool-key-false"], @(false)); | ||
| } | ||
|
|
||
| - (void)testSetDoubleAttribute | ||
| { | ||
| SentryScope *scope = [[SentryScope alloc] init]; | ||
| [scope setAttributeValue:@(1.4728) forKey:@"a-double-key"]; | ||
| XCTAssertEqualObjects([scope attributes][@"a-double-key"], @(1.4728)); | ||
| } | ||
|
|
||
| - (void)testSetIntegerAttribute | ||
| { | ||
| SentryScope *scope = [[SentryScope alloc] init]; | ||
| [scope setAttributeValue:@(4) forKey:@"an-integer-key"]; | ||
| XCTAssertEqualObjects([scope attributes][@"an-integer-key"], @(4)); | ||
| } | ||
|
|
||
| - (void)testRemoveAttribute | ||
| { | ||
| SentryScope *scope = [[SentryScope alloc] init]; | ||
| [scope setAttributeValue:@"test-string" forKey:@"a-key"]; | ||
| XCTAssertEqualObjects([scope attributes][@"a-key"], @"test-string"); | ||
|
|
||
| [scope removeAttributeForKey:@"a-key"]; | ||
| XCTAssertEqualObjects([scope attributes][@"a-key"], nil); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m: Please migrate these to SentryScopeSwiftTests instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved
| /** | ||
| * Set global attributes. Attributes are searchable key/value string pairs attached to every log | ||
| * message. | ||
| */ | ||
| - (void)setAttributeValue:(id)value forKey:(NSString *)key NS_SWIFT_NAME(setAttribute(value:key:)); | ||
|
|
||
| /** | ||
| * Remove the attribute for the specified key. | ||
| */ | ||
| - (void)removeAttributeForKey:(NSString *)key NS_SWIFT_NAME(removeAttribute(key:)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
h: We have to be very clear that the attributes are only applied to logs and in the future metrics and not for the rest, such as events, transactions, spans, profiles, SR. The same applies to the property above.
Sources/Sentry/SentryScope.m
Outdated
|
|
||
| - (void)setAttributeValue:(id)value forKey:(NSString *)key | ||
| { | ||
| if (key == nil || key.length == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m: You can set an empty key to a dict. Strictly speaking, it works.
| if (key == nil || key.length == 0) { | |
| if (key == nil) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but do they make sense as attributes?
@cleptric
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talked with him, they cannot be empty, will add to the docs
Sources/Sentry/SentryScope.m
Outdated
| return [self.propagationContext.traceId sentryIdString]; | ||
| } | ||
|
|
||
| - (NSDictionary<NSString *, id> *)attributes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: I would move all these methods a bit up in the class and put it below - (NSArray<SentryAttachment *> *)attachments. These are public methods and I think they should be close to the other public methods.
Sources/Sentry/SentryScope.m
Outdated
| // ScopeObservers are not called since at this moment attributes are only used for Logs, | ||
| // which the LogBatcher obtains manually. At this moment not even Spans use this attributes. | ||
| // Should this change, we will need to call the observers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m: The key difference is that the scope observes are used for watchdog terminations and crashes. As the SDK creates these events when a crash occurs or the next time the app launches for watchdog terminations, the scope data in memory would be lost. Logs instead are created on the fly. For logs we must ensure we don't loose the logs in memory for crashes and watchdog terminations, which we don't solve with scope observers, but in the batch processor instead by implementing a double rotating buffer: https://develop.sentry.dev/sdk/telemetry/telemetry-buffer/mobile-telemetry-buffer/#3-doublerotatingbuffer. So even for spans we won't change this. We could already put this info in this comment that we don't need to sync these attributes to the scope observers.
Sources/Sentry/SentryScope.m
Outdated
| [_attributesDictionary removeObjectForKey:key]; | ||
|
|
||
| // ScopeObservers are not called since at this moment attributes are only used for Logs, | ||
| // which the LogBatcher obtains manually. At this moment not even Spans use this attributes. | ||
| // Should this change, we will need to call the observers. | ||
| } | ||
| } | ||
|
|
||
| @end | ||
|
|
||
| NS_ASSUME_NONNULL_END |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
h: Yes, I think that we must remove the attributes also when calling scope.clear. I opened a develop docs PR to clarify that getsentry/sentry-docs#15578, but I'm not 100% it's expected behaviour. Let's wait for Michi to confirm.
| self.contextDictionary = [NSMutableDictionary new]; | ||
| self.attachmentArray = [NSMutableArray new]; | ||
| self.fingerprintArray = [NSMutableArray new]; | ||
| self.attributesDictionary = [NSMutableDictionary new]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
h: The serialize method doesn't add the attributes. I just realized it doesn't make any sense that the scope implements the SentrySerializable protocol, so we could actually still ditch it in V9. If not, we should add the attributes to the serialize method, because otherwise it's quite confusing.
Adds
setAttributeandremoveAttributetoSentryScopeas per https://develop.sentry.dev/sdk/telemetry/scopes/#scope-methodsThese attributes will be used for logs in a following PR.
Currently units are not supported since Logs don't support them (yet).
Closes #6835