-
-
Notifications
You must be signed in to change notification settings - Fork 461
Add span based feature flags #4831
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
Co-authored-by: Lorenzo Cian <17258265+lcian@users.noreply.github.com>
|
Bug: Feature Flag Data Handling Causes Unintended MutationsThe feature flag logic in the |
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 806307f | 357.85 ms | 424.64 ms | 66.79 ms |
| ee747ae | 554.98 ms | 611.50 ms | 56.52 ms |
| 17a0955 | 372.53 ms | 446.70 ms | 74.17 ms |
| 2124a46 | 319.19 ms | 415.04 ms | 95.85 ms |
| 9fbb112 | 404.51 ms | 475.65 ms | 71.14 ms |
| b6702b0 | 395.86 ms | 409.98 ms | 14.12 ms |
| ee747ae | 357.79 ms | 421.84 ms | 64.05 ms |
| 27d7cf8 | 314.17 ms | 347.00 ms | 32.83 ms |
| 9fbb112 | 361.43 ms | 427.57 ms | 66.14 ms |
| 96449e8 | 361.30 ms | 423.39 ms | 62.09 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 806307f | 1.58 MiB | 2.10 MiB | 533.42 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| 17a0955 | 1.58 MiB | 2.10 MiB | 533.20 KiB |
| 2124a46 | 1.58 MiB | 2.12 MiB | 551.51 KiB |
| 9fbb112 | 1.58 MiB | 2.11 MiB | 539.18 KiB |
| b6702b0 | 1.58 MiB | 2.12 MiB | 551.79 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| 27d7cf8 | 1.58 MiB | 2.12 MiB | 549.42 KiB |
| 9fbb112 | 1.58 MiB | 2.11 MiB | 539.18 KiB |
| 96449e8 | 1.58 MiB | 2.11 MiB | 539.35 KiB |
Previous results on branch: feat/feature-flags-on-spans
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ef06876 | 312.94 ms | 370.33 ms | 57.39 ms |
| cb9cf5e | 313.21 ms | 359.20 ms | 46.00 ms |
| 8f87720 | 367.76 ms | 460.02 ms | 92.26 ms |
| 8a5f7d1 | 322.31 ms | 431.93 ms | 109.63 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ef06876 | 1.58 MiB | 2.12 MiB | 553.23 KiB |
| cb9cf5e | 1.58 MiB | 2.12 MiB | 549.76 KiB |
| 8f87720 | 1.58 MiB | 2.12 MiB | 549.76 KiB |
| 8a5f7d1 | 1.58 MiB | 2.12 MiB | 549.68 KiB |
lbloder
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.
Nice work! Added some minor comments/questions
sentry/src/test/java/io/sentry/featureflags/SpanFeatureFlagBufferTest.kt
Outdated
Show resolved
Hide resolved
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: Feature Flags Vanish During Context Duplication
The SpanContext copy constructor doesn't copy the newly added featureFlags field. When contexts are copied through Contexts or MonitorContexts constructors, feature flags added to the span will be lost. This causes the copied SpanContext to have an empty feature flag buffer instead of preserving the flags from the source.
sentry/src/main/java/io/sentry/SpanContext.java#L120-L148
sentry-java/sentry/src/main/java/io/sentry/SpanContext.java
Lines 120 to 148 in 36be3b2
| */ | |
| public SpanContext(final @NotNull SpanContext spanContext) { | |
| this.traceId = spanContext.traceId; | |
| this.spanId = spanContext.spanId; | |
| this.parentSpanId = spanContext.parentSpanId; | |
| setSamplingDecision(spanContext.samplingDecision); | |
| this.op = spanContext.op; | |
| this.description = spanContext.description; | |
| this.status = spanContext.status; | |
| final Map<String, String> copiedTags = CollectionUtils.newConcurrentHashMap(spanContext.tags); | |
| if (copiedTags != null) { | |
| this.tags = copiedTags; | |
| } | |
| final Map<String, Object> copiedUnknown = | |
| CollectionUtils.newConcurrentHashMap(spanContext.unknown); | |
| if (copiedUnknown != null) { | |
| this.unknown = copiedUnknown; | |
| } | |
| this.baggage = spanContext.baggage; | |
| final Map<String, Object> copiedData = CollectionUtils.newConcurrentHashMap(spanContext.data); | |
| if (copiedData != null) { | |
| this.data = copiedData; | |
| } | |
| } | |
| public void setOperation(final @NotNull String operation) { | |
| this.op = Objects.requireNonNull(operation, "operation is required"); | |
| } | |
sentry-system-test-support/src/main/kotlin/io/sentry/systemtest/util/TestHelper.kt
Show resolved
Hide resolved
| transaction, | ||
| op = "spanCreatedThroughSentryApi", | ||
| featureFlag = FeatureFlag("flag.evaluation.my-feature-flag", true), | ||
| ) |
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: Feature Flags Attach to Wrong Active Span.
The test expects transaction-feature-flag on the transaction and my-feature-flag on the child span, but both flags are added using Sentry.addFeatureFlag() which writes to both scope and the current active span. When transaction-feature-flag is added before starting the child span, the current span is the transaction (root span). When my-feature-flag is added inside the child span, the current span is still the parent (since startChild() doesn't automatically make the child current unless ScopeBindingMode.ON is set). Both flags should end up on the transaction, not split between transaction and child span.
lbloder
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 👍
* Copy active span when cloning scope * changelog
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: Context Cloning: Feature Flags Vanish
The SpanContext copy constructor doesn't copy the featureFlags field, causing feature flags to be lost when contexts are cloned (e.g., when Scope is cloned via new Contexts(scope.contexts)). This results in feature flag data being discarded unexpectedly since featureFlags will be initialized to a fresh empty buffer instead of preserving the original flags.
sentry/src/main/java/io/sentry/SpanContext.java#L120-L141
sentry-java/sentry/src/main/java/io/sentry/SpanContext.java
Lines 120 to 141 in e10c410
| */ | |
| public SpanContext(final @NotNull SpanContext spanContext) { | |
| this.traceId = spanContext.traceId; | |
| this.spanId = spanContext.spanId; | |
| this.parentSpanId = spanContext.parentSpanId; | |
| setSamplingDecision(spanContext.samplingDecision); | |
| this.op = spanContext.op; | |
| this.description = spanContext.description; | |
| this.status = spanContext.status; | |
| final Map<String, String> copiedTags = CollectionUtils.newConcurrentHashMap(spanContext.tags); | |
| if (copiedTags != null) { | |
| this.tags = copiedTags; | |
| } | |
| final Map<String, Object> copiedUnknown = | |
| CollectionUtils.newConcurrentHashMap(spanContext.unknown); | |
| if (copiedUnknown != null) { | |
| this.unknown = copiedUnknown; | |
| } | |
| this.baggage = spanContext.baggage; | |
| final Map<String, Object> copiedData = CollectionUtils.newConcurrentHashMap(spanContext.data); | |
| if (copiedData != null) { | |
| this.data = copiedData; |
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: Flags Lost in Context Duplication
The SpanContext copy constructor doesn't copy the featureFlags field, causing feature flags to be lost when contexts are cloned (e.g., via Contexts or MonitorContexts copy constructors). The field should be cloned similar to how tags and data are copied.
sentry/src/main/java/io/sentry/SpanContext.java#L120-L144
sentry-java/sentry/src/main/java/io/sentry/SpanContext.java
Lines 120 to 144 in f07c6d1
| */ | |
| public SpanContext(final @NotNull SpanContext spanContext) { | |
| this.traceId = spanContext.traceId; | |
| this.spanId = spanContext.spanId; | |
| this.parentSpanId = spanContext.parentSpanId; | |
| setSamplingDecision(spanContext.samplingDecision); | |
| this.op = spanContext.op; | |
| this.description = spanContext.description; | |
| this.status = spanContext.status; | |
| final Map<String, String> copiedTags = CollectionUtils.newConcurrentHashMap(spanContext.tags); | |
| if (copiedTags != null) { | |
| this.tags = copiedTags; | |
| } | |
| final Map<String, Object> copiedUnknown = | |
| CollectionUtils.newConcurrentHashMap(spanContext.unknown); | |
| if (copiedUnknown != null) { | |
| this.unknown = copiedUnknown; | |
| } | |
| this.baggage = spanContext.baggage; | |
| final Map<String, Object> copiedData = CollectionUtils.newConcurrentHashMap(spanContext.data); | |
| if (copiedData != null) { | |
| this.data = copiedData; | |
| } | |
| } | |
📜 Description
Add
addFeatureFlagmethod to span and transaction.It'll allow setting a max of 10 feature flag evaluations per span.
Spans do not inherit or share flags, so each span is separate.
When the limit has been reached new values are rejected but existing values may be updated.
💡 Motivation and Context
Another part of #4763
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps