-
-
Notifications
You must be signed in to change notification settings - Fork 461
Add scope based feature flags #4812
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
|
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-scope
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 79c6335 | 342.31 ms | 399.16 ms | 56.85 ms |
| 0983aa0 | 369.83 ms | 412.26 ms | 42.43 ms |
| 3128a6f | 319.66 ms | 386.50 ms | 66.84 ms |
| 8dfdb03 | 349.64 ms | 423.82 ms | 74.18 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 79c6335 | 1.58 MiB | 2.12 MiB | 553.10 KiB |
| 0983aa0 | 1.58 MiB | 2.11 MiB | 540.70 KiB |
| 3128a6f | 1.58 MiB | 2.12 MiB | 553.11 KiB |
| 8dfdb03 | 1.58 MiB | 2.12 MiB | 549.53 KiB |
| tmpList.remove(0); | ||
| } | ||
|
|
||
| flags = new CopyOnWriteArrayList<>(tmpList); |
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.
Does it need to be CopyOnWrite if we never actually call flags.add()?
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.
I opted for CopyOnWriteArrayList due to it being a good choice for scope cloning without affecting parent scopes.
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.
Gotta think about whether using something else would work too, probably yes but what's the benefit?
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.
Should be possible to replace with ArrayList but then we must never modify the collection. I was still hoping for some enlightenment on how to speed up the add method. I've written this down and depending on how the add method turns out, we can replace it.
We could in theory wrap it with Collections.unmodifyableList but I'd rather send corrupted data as opposed to crashing the customers application.
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.
but then we must never modify the collection.
why is that? sorry for the questions I guess i'm missing some context 😅
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.
Discussed in more detail internally.
We currently optimized feature flags storage for scope cloning performance (where CopyOnWriteArrayList simply re-uses the internal array, when cloning the list, which is O(1)).
Merging was our second priority.
Add is currently rather inefficient.
We wanna release as is and if customers request better performance, we can revisit and create an Android specific implementation, that prioritizes differently and/or optimizes for fewer allocations.
| private @NotNull String flag; | ||
|
|
||
| /** Evaluation result of the feature flag. */ | ||
| private boolean result; |
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.
Flag evaluation is allowed to be an arbitrary JSON according to Relay https://github.com/getsentry/relay/blob/21c2e18ebe6f834a4ce4e149c6a43e4bec1e90f8/relay-event-schema/src/protocol/contexts/flags.rs#L30
However in docs and frontend this is typed as bool.
I think it will be easy to extend this with an overload that takes Object or we just change this to take it into account from the get go.
Let's also double check with @cmanallen on how to proceed here.
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.
Relay is set up to accommodate remote-configs/feature-flags of any JSON type. The UI currently expects boolean though. I'm not sure how resilient it is to non-boolean types. The goal is to expand our UI to accommodate more types in the future but AFAIK that's not been realized yet. Something I can find out later today.
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.
@cmanallen can we release this as is with Boolean param exposed to customers?
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.
Checked internally, Boolean is OK for now.
| final int size = flags.size(); | ||
| final @NotNull ArrayList<FeatureFlagEntry> tmpList = new ArrayList<>(size + 1); | ||
| for (FeatureFlagEntry entry : flags) { | ||
| if (!entry.flag.equals(flag)) { | ||
| tmpList.add(entry); | ||
| } | ||
| } | ||
| tmpList.add(new FeatureFlagEntry(flag, result, System.nanoTime())); | ||
|
|
||
| if (tmpList.size() > maxSize) { | ||
| tmpList.remove(0); | ||
| } | ||
|
|
||
| flags = new CopyOnWriteArrayList<>(tmpList); |
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.
| final int size = flags.size(); | |
| final @NotNull ArrayList<FeatureFlagEntry> tmpList = new ArrayList<>(size + 1); | |
| for (FeatureFlagEntry entry : flags) { | |
| if (!entry.flag.equals(flag)) { | |
| tmpList.add(entry); | |
| } | |
| } | |
| tmpList.add(new FeatureFlagEntry(flag, result, System.nanoTime())); | |
| if (tmpList.size() > maxSize) { | |
| tmpList.remove(0); | |
| } | |
| flags = new CopyOnWriteArrayList<>(tmpList); | |
| final int size = flags.size(); | |
| for (int i = 0; i < size; i++) { | |
| if (flags.get(i).equals(flag)) { | |
| flags.remove(i); | |
| break; | |
| } | |
| } | |
| flags.add(new FeatureFlagEntry(flag, result, System.nanoTime())); | |
| if (flags.size() > maxSize) { | |
| flags.remove(0); | |
| } |
Maybe this is better?
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!
Yes, it's better, see #4812 (comment) and #4812 (comment)
This is called "Mutable CopyOnWriteArrayList" in the tables.
|
Table compares ops/s for
Here's the code I used to benchmark: |
|
Reran benchmarks for a (hopefully) more realistic scenario NOTE: I just updated these after fixing a bug with the duplicate check
Added a new implementation which looks for the duplicate flag in reverse order: |
| featureFlags.add(entry.toFeatureFlag()); | ||
| } | ||
| return new FeatureFlags(featureFlags); | ||
| } |
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: Return null when feature flags are empty
The getFeatureFlags() method always returns a non-null FeatureFlags object even when the buffer is empty (containing zero flags). This causes unnecessary bloat in event payloads because an empty FeatureFlags object with an empty list will be added to events even when no feature flags have been recorded. The method should return null when the flags list is empty to avoid adding empty FeatureFlags objects to events, similar to how NoOpFeatureFlagBuffer.getFeatureFlags() returns null.
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: Contexts Cloning: FeatureFlags State Shared
The Contexts copy constructor doesn't handle FeatureFlags type when cloning contexts. All other context types like App, Browser, Device, Spring, etc. have explicit copy constructor handling, but FeatureFlags is missing. This causes FeatureFlags to fall through to the else branch where it gets shallow copied instead of deep copied, potentially leading to shared mutable state between the original and cloned contexts.
sentry/src/main/java/io/sentry/protocol/Contexts.java#L39-L72
sentry-java/sentry/src/main/java/io/sentry/protocol/Contexts.java
Lines 39 to 72 in 7c4449a
| public Contexts(final @NotNull Contexts contexts) { | |
| for (final Map.Entry<String, Object> entry : contexts.entrySet()) { | |
| if (entry != null) { | |
| final Object value = entry.getValue(); | |
| if (App.TYPE.equals(entry.getKey()) && value instanceof App) { | |
| this.setApp(new App((App) value)); | |
| } else if (Browser.TYPE.equals(entry.getKey()) && value instanceof Browser) { | |
| this.setBrowser(new Browser((Browser) value)); | |
| } else if (Device.TYPE.equals(entry.getKey()) && value instanceof Device) { | |
| this.setDevice(new Device((Device) value)); | |
| } else if (OperatingSystem.TYPE.equals(entry.getKey()) | |
| && value instanceof OperatingSystem) { | |
| this.setOperatingSystem(new OperatingSystem((OperatingSystem) value)); | |
| } else if (SentryRuntime.TYPE.equals(entry.getKey()) && value instanceof SentryRuntime) { | |
| this.setRuntime(new SentryRuntime((SentryRuntime) value)); | |
| } else if (Feedback.TYPE.equals(entry.getKey()) && value instanceof Feedback) { | |
| this.setFeedback(new Feedback((Feedback) value)); | |
| } else if (Gpu.TYPE.equals(entry.getKey()) && value instanceof Gpu) { | |
| this.setGpu(new Gpu((Gpu) value)); | |
| } else if (SpanContext.TYPE.equals(entry.getKey()) && value instanceof SpanContext) { | |
| this.setTrace(new SpanContext((SpanContext) value)); | |
| } else if (ProfileContext.TYPE.equals(entry.getKey()) && value instanceof ProfileContext) { | |
| this.setProfile(new ProfileContext((ProfileContext) value)); | |
| } else if (Response.TYPE.equals(entry.getKey()) && value instanceof Response) { | |
| this.setResponse(new Response((Response) value)); | |
| } else if (Spring.TYPE.equals(entry.getKey()) && value instanceof Spring) { | |
| this.setSpring(new Spring((Spring) value)); | |
| } else { | |
| this.put(entry.getKey(), value); | |
| } | |
| } | |
| } | |
| } |
| FeatureFlags(final @NotNull FeatureFlags featureFlags) { | ||
| this.values = featureFlags.values; | ||
| this.unknown = CollectionUtils.newConcurrentHashMap(featureFlags.unknown); | ||
| } |
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: Shallow Copy Breaks FeatureFlags Isolation
The FeatureFlags copy constructor performs a shallow copy by directly assigning this.values = featureFlags.values, causing both the original and copied FeatureFlags instances to share the same List object. Modifications to the list in one instance will be visible in the other, breaking the expected isolation between copied objects. Should create a new list instance containing the same elements.
📜 Description
addFeatureFlagcan be used to track feature flag evaluations and have them show up on errors in Sentry💡 Motivation and Context
Scope based part of #4763
💚 How did you test it?
Manually + E2E tests + unit tests
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
sentry.properties)