-
-
Notifications
You must be signed in to change notification settings - Fork 376
fix(logs): Use sendDefaultPii and span_id for attributes #7055
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7055 +/- ##
=============================================
- Coverage 84.947% 84.931% -0.016%
=============================================
Files 457 457
Lines 27604 27608 +4
Branches 12139 12140 +1
=============================================
- Hits 23449 23448 -1
- Misses 4114 4119 +5
Partials 41 41
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
itaybre
left a comment
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.
LGTM, with a small comment
Pull request was converted to draft
|
Moving back to draft until I find a way to do this without adding public API |
|
I found the rather obvious way of adding it to the BatcherConfig (not BatcherMetadata even though it originates from the options, but isn't additional metadata). Now there's on change in public API. |
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: User id still sent when PII disabled
addUserAttributes now skips user.* when config.sendDefaultPii is false, but addDefaultUserIdIfNeeded can still add user.id (from metadata.installationId). This means logs can still contain a stable user identifier even when sendDefaultPii is disabled, and can replace an explicitly set user with the installation id.
Sources/Swift/Tools/Batcher/BatcherScope.swift#L73-L87
sentry-cocoa/Sources/Swift/Tools/Batcher/BatcherScope.swift
Lines 73 to 87 in 872c2d4
| private func addUserAttributes(to attributes: inout [String: SentryAttribute], config: any BatcherConfig) { | |
| guard config.sendDefaultPii else { | |
| return | |
| } | |
| if let userId = userObject?.userId { | |
| attributes["user.id"] = .init(string: userId) | |
| } | |
| if let userName = userObject?.name { | |
| attributes["user.name"] = .init(string: userName) | |
| } | |
| if let userEmail = userObject?.email { | |
| attributes["user.email"] = .init(string: userEmail) | |
| } | |
| } |
Sources/Swift/Tools/Batcher/BatcherScope.swift#L106-L121
sentry-cocoa/Sources/Swift/Tools/Batcher/BatcherScope.swift
Lines 106 to 121 in 872c2d4
| private func addDefaultUserIdIfNeeded( | |
| to attributes: inout [String: SentryAttribute], | |
| config: any BatcherConfig, | |
| metadata: any BatcherMetadata | |
| ) { | |
| guard attributes["user.id"] == nil && attributes["user.name"] == nil && attributes["user.email"] == nil else { | |
| return | |
| } | |
| if let installationId = metadata.installationId { | |
| // We only want to set the id if the customer didn't set a user so we at least set something to | |
| // identify the user. | |
| attributes["user.id"] = .init(value: installationId) | |
| } | |
| } |
Bug: Test still asserts old span attribute
testAddLog_DoesNotAddNilDefaultAttributes still checks attributes["sentry.trace.parent_span_id"] after the production code switched the default attribute key to span_id, so the test no longer matches the new behavior and can fail or miss regressions around the new key.
Tests/SentryTests/SentryLogBatcherTests.swift#L330-L351
sentry-cocoa/Tests/SentryTests/SentryLogBatcherTests.swift
Lines 330 to 351 in 872c2d4
| func testAddLog_DoesNotAddNilDefaultAttributes() throws { | |
| // -- Arrange -- | |
| options.releaseName = nil | |
| let sut = getSut() | |
| let log = createTestLog(body: "Test log message") | |
| // -- Act -- | |
| sut.addLog(log, scope: scope) | |
| sut.captureLogs() | |
| // -- Assert -- | |
| let capturedLogs = testDelegate.getCapturedLogs() | |
| let capturedLog = try XCTUnwrap(capturedLogs.first) | |
| let attributes = capturedLog.attributes | |
| XCTAssertNil(attributes["sentry.release"]) | |
| XCTAssertNil(attributes["sentry.trace.parent_span_id"]) | |
| XCTAssertEqual(attributes["sentry.sdk.name"]?.value as? String, SentryMeta.sdkName) | |
| XCTAssertEqual(attributes["sentry.sdk.version"]?.value as? String, SentryMeta.versionString) | |
| XCTAssertNotNil(attributes["sentry.environment"]) | |
| } |
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: User ID fallback ignores sendDefaultPii
addUserAttributes now correctly guards user.* fields behind config.sendDefaultPii, but addDefaultUserIdIfNeeded still writes attributes["user.id"] from metadata.installationId even when sendDefaultPii is false. This can still emit a user-identifying attribute despite opting out of default PII.
Sources/Swift/Tools/Batcher/BatcherScope.swift#L73-L121
sentry-cocoa/Sources/Swift/Tools/Batcher/BatcherScope.swift
Lines 73 to 121 in f8dd108
| private func addUserAttributes(to attributes: inout [String: SentryAttribute], config: any BatcherConfig) { | |
| guard config.sendDefaultPii else { | |
| return | |
| } | |
| if let userId = userObject?.userId { | |
| attributes["user.id"] = .init(string: userId) | |
| } | |
| if let userName = userObject?.name { | |
| attributes["user.name"] = .init(string: userName) | |
| } | |
| if let userEmail = userObject?.email { | |
| attributes["user.email"] = .init(string: userEmail) | |
| } | |
| } | |
| private func addReplayAttributes(to attributes: inout [String: SentryAttribute], config: any BatcherConfig) { | |
| #if canImport(UIKit) && !SENTRY_NO_UIKIT | |
| #if os(iOS) || os(tvOS) | |
| if let scopeReplayId = replayId { | |
| // Session mode: use scope replay ID | |
| attributes["sentry.replay_id"] = .init(string: scopeReplayId) | |
| } | |
| #endif | |
| #endif | |
| } | |
| private func addScopeAttributes(to attributes: inout [String: SentryAttribute], config: any BatcherConfig) { | |
| // Scope attributes should not override any existing attribute in the item | |
| for (key, value) in self.attributes where attributes[key] == nil { | |
| attributes[key] = .init(value: value) | |
| } | |
| } | |
| private func addDefaultUserIdIfNeeded( | |
| to attributes: inout [String: SentryAttribute], | |
| config: any BatcherConfig, | |
| metadata: any BatcherMetadata | |
| ) { | |
| guard attributes["user.id"] == nil && attributes["user.name"] == nil && attributes["user.email"] == nil else { | |
| return | |
| } | |
| if let installationId = metadata.installationId { | |
| // We only want to set the id if the customer didn't set a user so we at least set something to | |
| // identify the user. | |
| attributes["user.id"] = .init(value: installationId) | |
| } | |
| } |
philipphofmann
left a comment
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.
Way better than before, thanks 💯
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 6d0b605 | 1218.58 ms | 1251.06 ms | 32.48 ms |
| 854ca12 | 1219.94 ms | 1251.32 ms | 31.38 ms |
| 3279d4e | 1215.76 ms | 1256.45 ms | 40.69 ms |
| a9fac2e | 1212.45 ms | 1219.67 ms | 7.22 ms |
| ebc72be | 1221.24 ms | 1249.66 ms | 28.42 ms |
| 7c7ac56 | 1225.90 ms | 1250.22 ms | 24.33 ms |
| 67e8e3e | 1220.08 ms | 1229.23 ms | 9.15 ms |
| be9107c | 1223.63 ms | 1242.82 ms | 19.19 ms |
| 2b02431 | 1229.63 ms | 1248.98 ms | 19.35 ms |
| 5b44596 | 1220.87 ms | 1247.27 ms | 26.40 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 6d0b605 | 23.75 KiB | 1023.82 KiB | 1000.07 KiB |
| 854ca12 | 23.74 KiB | 996.96 KiB | 973.22 KiB |
| 3279d4e | 23.75 KiB | 938.32 KiB | 914.57 KiB |
| a9fac2e | 23.75 KiB | 879.53 KiB | 855.78 KiB |
| ebc72be | 23.75 KiB | 908.22 KiB | 884.47 KiB |
| 7c7ac56 | 23.75 KiB | 902.49 KiB | 878.74 KiB |
| 67e8e3e | 23.75 KiB | 919.91 KiB | 896.16 KiB |
| be9107c | 23.75 KiB | 975.19 KiB | 951.44 KiB |
| 2b02431 | 23.75 KiB | 850.73 KiB | 826.98 KiB |
| 5b44596 | 23.75 KiB | 1.00 MiB | 1002.41 KiB |
📜 Description
sentry.trace.parent_span_idtospan_idas defined in the Log ProtocolsendDefaultPiioption to the scope to use it for guarding of user-related attributes (as recently changed in the Log Protocol)💡 Motivation and Context
Closes #7054
💚 How did you test it?
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.