Skip to content
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

feat(flags): Add LaunchDarkly integration #14207

Open
wants to merge 62 commits into
base: develop
Choose a base branch
from

Conversation

aliu39
Copy link
Member

@aliu39 aliu39 commented Nov 7, 2024

Adds a browser integration for tracking feature flag evaluations through the LaunchDarkly JS SDK.

Notion doc summarizing our goal, constraints, and potential approaches: https://www.notion.so/sentry/Feature-Flags-JavaScript-SDK-1358b10e4b5d805288abe1a4fede75ed?pvs=4. This PR implements approach 1.

Also see https://develop.sentry.dev/sdk/expected-features/#feature-flags

Ref

Decisions:

  1. we are storing the flags in the current scope's context, and copying them to the event context on error. This way we don't have to extend the scope class/core package. Potential follow-ups:
  1. users have to register the LD hook (LDInspectionFlagUsedHandler) separately from the Sentry.init() call, since the only way to do so is in [LD].initialize().
  2. this hook is available for LD client SDKs only (JS, react, and node-client), hence why this is a browser only integration.
  3. To keep bundle sizes light, this integration is not exported to CDN bundles. Users should install with npm, and we expect tree-shaking to help if this integration is unused.

packages/types/src/context.ts Outdated Show resolved Hide resolved
@aliu39 aliu39 force-pushed the aliu/launch-darkly-integration branch from 2ba9592 to d8f2d72 Compare November 13, 2024 18:03
@aliu39 aliu39 force-pushed the aliu/launch-darkly-integration branch from d8f2d72 to 7a5f29d Compare November 13, 2024 18:29
yarn.lock Outdated Show resolved Hide resolved
@aliu39 aliu39 force-pushed the aliu/launch-darkly-integration branch from ab1bb31 to 8479826 Compare November 14, 2024 01:24
@aliu39 aliu39 force-pushed the aliu/launch-darkly-integration branch from 8479826 to a2ba257 Compare November 14, 2024 01:41
Comment on lines +25 to +33
await page.waitForFunction(bufferSize => {
const ldClient = (window as any).initializeLD();
for (let i = 1; i <= bufferSize; i++) {
ldClient.variation(`feat${i}`, false);
}
ldClient.variation(`feat${bufferSize + 1}`, true); // eviction
ldClient.variation('feat3', true); // update
return true;
}, FLAG_BUFFER_SIZE);
Copy link
Member

Choose a reason for hiding this comment

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

This can just be page.evaluate right since we're not waiting for anything async inside

Comment on lines +28 to +48
await page.waitForFunction(() => {
const Sentry = (window as any).Sentry;
const errorButton = document.querySelector('#error') as HTMLButtonElement;
const ldClient = (window as any).initializeLD();

ldClient.variation('shared', true);

Sentry.withScope((scope: Scope) => {
ldClient.variation('forked', true);
ldClient.variation('shared', false);
scope.setTag('isForked', true);
if (errorButton) {
errorButton.click();
}
});

ldClient.variation('main', true);
Sentry.getCurrentScope().setTag('isForked', false);
errorButton.click();
return true;
});
Copy link
Member

Choose a reason for hiding this comment

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

same here re: page.evaluate?

*
* @returns `true` if we should skip the launchdarkly test
*/
export function shouldSkipLaunchDarklyTest(): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be more generic? e.g. feature flags instead of just launch darkly, esp if we're going to add other vendors.

Suggested change
export function shouldSkipLaunchDarklyTest(): boolean {
export function shouldSkipLaunchDarklyTest(): boolean {

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it can be!

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.

7 participants