Skip to content

Conversation

@adinauer
Copy link
Member

@adinauer adinauer commented Oct 16, 2025

📜 Description

addFeatureFlag can 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

  • I added GH Issue ID & Linear ID
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

  • Test (de)serialization
  • Add maxFeatureFlags to external options (sentry.properties)
  • Add span based API
  • Make params on addFeatureFlag nullable

@github-actions
Copy link
Contributor

github-actions bot commented Oct 16, 2025

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 7c4449a

cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 16, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 306.06 ms 368.79 ms 62.73 ms
Size 1.58 MiB 2.12 MiB 555.24 KiB

Baseline results on branch: main

Startup times

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);
Copy link
Member

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()?

Copy link
Member Author

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.

Copy link
Member Author

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?

Copy link
Member Author

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.

Copy link
Member

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 😅

Copy link
Member Author

@adinauer adinauer Oct 20, 2025

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;
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member Author

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.

Comment on lines 43 to 56
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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?

Copy link
Member Author

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.

@adinauer
Copy link
Member Author

Table compares ops/s for add method of different implementations

Implementation Unique Duplicates Duplicates in Full Buffer
Immutable CopyOnWriteArrayList 74,071.034 363,250.814 29,828.637
Immutable ArrayList 67,329.692 425,473.814 26,404.218
Mutable CopyOnWriteArrayList 146,023.283 155,988.324 51,812.315
  • Immutable CopyOnWriteArrayList = current state in this PR, where we create a temporary ArrayList for creating the new state
            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);
  • Immutable ArrayList = replaced CopyOnWriteArrayList with ArrayList which we replace in add
            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 = tmpList;
  • Mutable CopyOnWriteArrayList = iterate flags, remove existing entry if found
            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);
            }

Here's the code I used to benchmark:

  @Benchmark
  public void addUniqueFeatureFlag(Blackhole blackhole) throws InterruptedException {
    IFeatureFlagBuffer buffer = FeatureFlagBufferCOWAL.create(100);

    for (int i = 0; i < 100; i++) {
      buffer.add("unique_" + i, true);
    }

    blackhole.consume(buffer);
  }

  @Benchmark
  public void addNonUniqueFeatureFlag(Blackhole blackhole) throws InterruptedException {
    IFeatureFlagBuffer buffer = FeatureFlagBufferCOWAL.create(100);

    for (int i = 0; i < 100; i++) {
      buffer.add("unique_" + 1, true);
    }

    blackhole.consume(buffer);
  }

  @Benchmark
  public void addNonUniqueToFullFeatureFlagBuffer(Blackhole blackhole) throws InterruptedException {
    IFeatureFlagBuffer buffer = FeatureFlagBufferCOWAL.create(100);

    for (int i = 0; i < 100; i++) {
      buffer.add("unique_" + i, true);
    }

    for (int i = 0; i < 100; i++) {
      buffer.add("unique_" + 1, true);
    }

    blackhole.consume(buffer);
  }

@adinauer
Copy link
Member Author

adinauer commented Oct 20, 2025

Reran benchmarks for a (hopefully) more realistic scenario

NOTE: I just updated these after fixing a bug with the duplicate check

Implementation Realistic Usage
Immutable CopyOnWriteArrayList 100,522.600
Immutable ArrayList 89,760.930
Mutable CopyOnWriteArrayList 135,685.613
Mutable CopyOnWriteArrayList (reverse) 133,285.356
  @Benchmark
  public void realisticFeatureFlagUsage(Blackhole blackhole) throws InterruptedException {
    IFeatureFlagBuffer buffer = FeatureFlagBufferCOWAL.create(100);

    for (int i = 0; i < 50; i++) {
      buffer.add("flag_" + i, i % 2 == 0);
    }

    int[] duplicateIndices = {25, 0, 49, 12, 37, 5, 44, 18, 31, 2, 47, 8, 41, 15, 28, 22, 35, 10, 39, 20};
    for (int i = 0; i < 20; i++) {
      buffer.add("flag_" + duplicateIndices[i], true);
    }

    for (int i = 50; i < 70; i++) {
      buffer.add("flag_" + i, i % 2 == 0);
    }

    blackhole.consume(buffer);
  }

Added a new implementation which looks for the duplicate flag in reverse order:

            final int size = flags.size();
            for (int i = size - 1; i >= 0; i--) {
        		final @NotNull FeatureFlagEntry entry = flags.get(i);
        		if (entry.flag.equals(flag)) {
                    flags.remove(i);
                    break;
                }
            }
            flags.add(new FeatureFlagEntry(flag, result, System.nanoTime()));

            if (flags.size() > maxSize) {
                flags.remove(0);
            }

cursor[bot]

This comment was marked as outdated.

featureFlags.add(entry.toFeatureFlag());
}
return new FeatureFlags(featureFlags);
}
Copy link

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.

Fix in Cursor Fix in Web

Copy link

@cursor cursor bot left a 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

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);
}
}
}
}

Fix in Cursor Fix in Web


FeatureFlags(final @NotNull FeatureFlags featureFlags) {
this.values = featureFlags.values;
this.unknown = CollectionUtils.newConcurrentHashMap(featureFlags.unknown);
}
Copy link

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.

Fix in Cursor Fix in Web

@adinauer adinauer merged commit 5d8a2df into main Nov 11, 2025
60 of 62 checks passed
@adinauer adinauer deleted the feat/feature-flags-on-scope branch November 11, 2025 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants