feat: Add span feature flag API#8158
Conversation
Add Swift APIs for recording feature flag evaluations on spans and transactions. Feature flags recorded through the scope are also forwarded to the currently active span, while preserving independent span buffers. Serialize span and transaction feature flags as trace data using the `flag.evaluation.<name>` convention.
|
|
This is a stacked PR, so we should not enable auto-merge on it. |
denrase
requested review from
NinjaLikesCheez,
itaybre,
noahsmartin,
philipphofmann and
philprime
as code owners
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
|
||
| - (void)addFeatureFlagWithName:(NSString *)name result:(BOOL)result | ||
| { | ||
| id<SentrySpan> activeSpan = self.span; |
There was a problem hiding this comment.
Bug: A race condition in addFeatureFlagWithName: can cause feature flags to be lost if they are added while a transaction is being finished and serialized.
Severity: MEDIUM
Suggested Fix
To prevent the race condition, ensure that the modification of the span's feature flags is atomic with respect to the span's serialization process. This could be achieved by moving the read of the activeSpan and its subsequent modification into a synchronized block that uses the same lock as the span's state management, or by adding a check to prevent modification if the span has already been serialized.
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#L773
Potential issue: A race condition can occur in `addFeatureFlagWithName:`. The active
span is read at the beginning of the method, but the feature flag is added to it later.
If another thread finishes and serializes the transaction in between these two actions,
the feature flag will be added to the span's buffer after the buffer has already been
serialized. This results in the feature flag data being lost and not included in the
final transaction event, leading to a silent data loss without causing a crash.
Also affects:
Sources/Sentry/SentryScope.m:787~787
Did we get this right? 👍 / 👎 to inform future reviews.
📲 Install BuildsiOS
|
Performance metrics 🚀
|
|
|
||
| - (void)addFeatureFlagWithName:(NSString *)name result:(BOOL)result | ||
| { | ||
| id<SentrySpan> activeSpan = self.span; |
There was a problem hiding this comment.
m: I also think the activeSpan must be synchronized
|
|
||
| - (void)removeFeatureFlagWithName:(NSString *)name | ||
| { | ||
| id<SentrySpan> activeSpan = self.span; |
|
|
||
| - (void)addFeatureFlagWithName:(NSString *)name result:(BOOL)result | ||
| { | ||
| [self.featureFlagBuffer addWithName:name result:result]; |
There was a problem hiding this comment.
m: Do we need to synchronize the feature flag buffer?
2 participants
📜 Description
Add Swift APIs for recording feature flag evaluations on spans and transactions.
Feature flags recorded through the scope are also forwarded to the currently active span, while preserving independent span buffers.
Serialize span and transaction feature flags as trace data using the
flag.evaluation.<name>convention.💡 Motivation and Context
Closes #7990
💚 How did you test it?
Unit tests. Sample app.
https://sentry-sdks.sentry.io/explore/traces/trace/6e743a46c18d4e7091d6465b949f82f2/?node=span-205484086d134b0c&project=-1&query=trace%3A6e743a46c18d4e7091d6465b949f82f2&source=traces&sta
tsPeriod=14d&targetId=205484086d134b0c
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.