-
-
Notifications
You must be signed in to change notification settings - Fork 372
fix: allow transaction tags to be accessed and modified in beforeSend #6910
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6910 +/- ##
=============================================
- Coverage 85.894% 84.917% -0.977%
=============================================
Files 453 453
Lines 37531 27609 -9922
Branches 17430 12125 -5305
=============================================
- Hits 32237 23445 -8792
+ Misses 5251 4118 -1133
- Partials 43 46 +3
... and 414 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
47ffab3 to
94f3691
Compare
Override tags property in SentryTransaction to merge event tags and tracer tags, allowing full CRUD operations (Read, Add, Modify, Remove, Replace) in beforeSend. - Getter merges event tags and tracer tags (tracer takes precedence) - Setter clears both event and tracer tags, then sets all new tags on tracer - Added comprehensive tests for all tag operations in beforeSend - Added tests verifying Swift's automatic setter behavior with dictionary subscript assignment Fixes #5308
Add documentation note explaining special behavior of tags property for SentryTransaction instances in beforeSend callbacks: - Tags property merges event and tracer tags (tracer takes precedence) - Swift automatically calls setter with dictionary subscript assignment - Objective-C requires explicit setter call to persist modifications
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 26f7b17 | 1218.47 ms | 1253.82 ms | 35.35 ms |
| 139db8b | 1231.50 ms | 1258.19 ms | 26.69 ms |
| 134fbdf | 1219.71 ms | 1240.35 ms | 20.64 ms |
| 83bb978 | 1238.33 ms | 1260.04 ms | 21.71 ms |
| 331dad6 | 1210.40 ms | 1242.06 ms | 31.67 ms |
| 85a741b | 1217.02 ms | 1239.27 ms | 22.25 ms |
| 083e8c5 | 1227.74 ms | 1262.37 ms | 34.62 ms |
| d7461dc | 1233.69 ms | 1255.29 ms | 21.60 ms |
| 939d583 | 1209.96 ms | 1251.09 ms | 41.13 ms |
| 5fcb6a1 | 1198.86 ms | 1226.89 ms | 28.03 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 26f7b17 | 23.75 KiB | 960.93 KiB | 937.19 KiB |
| 139db8b | 23.75 KiB | 920.64 KiB | 896.89 KiB |
| 134fbdf | 23.75 KiB | 875.25 KiB | 851.50 KiB |
| 83bb978 | 23.75 KiB | 920.64 KiB | 896.89 KiB |
| 331dad6 | 23.75 KiB | 928.12 KiB | 904.37 KiB |
| 85a741b | 23.75 KiB | 959.44 KiB | 935.69 KiB |
| 083e8c5 | 23.75 KiB | 981.75 KiB | 958.00 KiB |
| d7461dc | 23.75 KiB | 874.45 KiB | 850.70 KiB |
| 939d583 | 23.75 KiB | 1023.82 KiB | 1000.07 KiB |
| 5fcb6a1 | 24.14 KiB | 1.01 MiB | 1014.60 KiB |
Previous results on branch: philprime/beforesend-transactions
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d9306d2 | 1232.59 ms | 1258.87 ms | 26.28 ms |
| 6129c9a | 1216.77 ms | 1251.13 ms | 34.37 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d9306d2 | 24.14 KiB | 1.01 MiB | 1012.89 KiB |
| 6129c9a | 24.14 KiB | 1.01 MiB | 1013.15 KiB |
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
Description
Fixes #5308
This PR implements getter and setter overrides for the
tagsproperty inSentryTransactionto allow full CRUD operations (Read, Add, Modify, Remove, Replace) on transaction tags withinbeforeSendcallbacks.See this issue comment for a full explanation reproducer.
Problem
When accessing
transaction.tagsinbeforeSend, it only returned tags from the superclassSentryEvent, missing tags set on the underlyingSentryTracer. Additionally, modifications totransaction.tagsinbeforeSenddidn't persist because they only affected the event tags, not the tracer tags that are actually serialized.Solution Approach
We implemented a computed property pattern that:
Getter: Merges tags from both
SentryEvent(superclass) andSentryTracer(wrapped tracer), with tracer tags taking precedenceSetter: Replaces all tags by:
Implementation Details
Swift vs Objective-C Behavior
Swift: When code does
transaction.tags?["key"] = "value", Swift automatically expands this to get tags, modify copy, and call the setter with the modified copy. This means modifications persist automatically.Objective-C: Modifying the returned dictionary directly does NOT persist. Users must explicitly call the setter:
transaction.tags = modifiedDict.This difference is documented in code comments to help developers understand the behavior in both languages.
Testing
Added comprehensive tests covering all CRUD operations:
All tests verify behavior both in
beforeSendcallbacks and in the final serialized output.Alternatives Considered
1. Custom NSMutableDictionary Subclass
Approach: Create a custom subclass that automatically calls the setter on every mutation.
Why Not Implemented:
setObject:forKey:,removeObjectForKey:,removeAllObjects, etc.)2. Proxy Pattern with Method Swizzling
Approach: Use method swizzling or a proxy to intercept dictionary mutations.
Why Not Implemented:
3. Direct Tracer Access
Approach: Document that users should access
transaction.trace.tagsdirectly.Why Not Implemented:
Changes
tagsgetter override inSentryTransaction.mthat merges event and tracer tagstagssetter override inSentryTransaction.mthat replaces all tags on the tracerSentryTransactionTests.swiftfor all tag operationsSentryClientTests.swiftverifying tag operations work inbeforeSendcallbacksTesting
beforeSendcallbacks in sample apps