From d45bf81659c4b1288b16eebe99aaeaad508ba2ad Mon Sep 17 00:00:00 2001 From: Andrew Liu <159852527+aliu39@users.noreply.github.com> Date: Tue, 10 Dec 2024 17:26:45 -0800 Subject: [PATCH 1/4] ref(flags): instrument sdk featureFlagsIntegration to track FE flag evals --- static/app/actionCreators/organization.tsx | 9 +- static/app/bootstrap/initializeSdk.tsx | 9 +- static/app/utils/featureFlags.ts | 73 ++++++ static/app/utils/featureObserver.spec.ts | 251 ------------------- static/app/utils/featureObserver.ts | 111 -------- static/app/views/projects/projectContext.tsx | 6 +- 6 files changed, 85 insertions(+), 374 deletions(-) create mode 100644 static/app/utils/featureFlags.ts delete mode 100644 static/app/utils/featureObserver.spec.ts delete mode 100644 static/app/utils/featureObserver.ts diff --git a/static/app/actionCreators/organization.tsx b/static/app/actionCreators/organization.tsx index e76f2c6e13a3a4..1ca0b216fc31e6 100644 --- a/static/app/actionCreators/organization.tsx +++ b/static/app/actionCreators/organization.tsx @@ -15,7 +15,10 @@ import TeamStore from 'sentry/stores/teamStore'; import type {Organization, Team} from 'sentry/types/organization'; import type {Project} from 'sentry/types/project'; import FeatureFlagOverrides from 'sentry/utils/featureFlagOverrides'; -import FeatureObserver from 'sentry/utils/featureObserver'; +import { + addOrganizationFeaturesHook, + getSentryFeaturesHook, +} from 'sentry/utils/featureFlags'; import {getPreloadedDataPromise} from 'sentry/utils/getPreloadedData'; import parseLinkHeader from 'sentry/utils/parseLinkHeader'; @@ -42,8 +45,10 @@ async function fetchOrg( } FeatureFlagOverrides.singleton().loadOrg(org); - FeatureObserver.singleton({}).observeOrganizationFlags({ + const sentryFeaturesHook = getSentryFeaturesHook(); + addOrganizationFeaturesHook({ organization: org, + hook: sentryFeaturesHook, }); OrganizationStore.onUpdate(org, {replace: true}); diff --git a/static/app/bootstrap/initializeSdk.tsx b/static/app/bootstrap/initializeSdk.tsx index ab3a7671a80e64..a840ea85528919 100644 --- a/static/app/bootstrap/initializeSdk.tsx +++ b/static/app/bootstrap/initializeSdk.tsx @@ -14,7 +14,6 @@ import { useNavigationType, } from 'react-router-dom'; import {useEffect} from 'react'; -import FeatureObserver from 'sentry/utils/featureObserver'; const SPA_MODE_ALLOW_URLS = [ 'localhost', @@ -72,6 +71,7 @@ function getSentryIntegrations() { filterKeys: ['sentry-spa'], behaviour: 'apply-tag-if-contains-third-party-frames', }), + Sentry.featureFlagsIntegration(), ]; return integrations; @@ -179,15 +179,8 @@ export function initializeSdk(config: Config) { handlePossibleUndefinedResponseBodyErrors(event); addEndpointTagToRequestError(event); - lastEventId = event.event_id || hint.event_id; - // attach feature flags to the event context - if (event.contexts) { - const flags = FeatureObserver.singleton({}).getFeatureFlags(); - event.contexts.flags = flags; - } - return event; }, }); diff --git a/static/app/utils/featureFlags.ts b/static/app/utils/featureFlags.ts new file mode 100644 index 00000000000000..10ccdb0f508a94 --- /dev/null +++ b/static/app/utils/featureFlags.ts @@ -0,0 +1,73 @@ +import {logger} from '@sentry/core'; +import * as Sentry from '@sentry/react'; + +import type {Organization} from 'sentry/types/organization'; +import type {Project} from 'sentry/types/project'; + +/** + * Returns a callback that can be used to track sentry flag evaluations through + * the Sentry SDK, in the event context. If the FeatureFlagsIntegration is not + * installed, the callback is a no-op. + */ +export function getSentryFeaturesHook(): (name: string, value: unknown) => void { + const featureFlagsIntegration = + Sentry.getClient()?.getIntegrationByName( + 'FeatureFlags' + ); + if (!featureFlagsIntegration || !('addFeatureFlag' in featureFlagsIntegration)) { + logger.error( + 'Unable to track flag evaluations because FeatureFlagsIntegration is not installed correctly.' + ); + return (_name, _value) => {}; + } + return (name: string, value: unknown) => { + // Append `feature.organizations:` in front to match the Sentry options automator format + featureFlagsIntegration?.addFeatureFlag('feature.organizations:' + name, value); + }; +} + +/** + * Registers a hook that processes feature names and values on each call to + * organization.features.includes(). + */ +export function addOrganizationFeaturesHook({ + organization, + hook, +}: { + hook: (name: string, value: unknown) => void; + organization: Organization; +}) { + const handler = { + apply: (target: any, orgFeatures: string[], flagName: string[]) => { + // Evaluate the result of .includes() + const flagResult = target.apply(orgFeatures, flagName); + hook(flagName[0], flagResult); + return flagResult; + }, + }; + const proxy = new Proxy(organization.features.includes, handler); + organization.features.includes = proxy; +} + +/** + * Registers a handler to track names and values of features passed into the + * project.features.includes() function. + */ +export function addProjectFeaturesHook({ + project, + hook, +}: { + hook: (name: string, value: unknown) => void; + project: Project; +}) { + const handler = { + apply: (target: any, projFeatures: string[], flagName: string[]) => { + // Evaluate the result of .includes() + const flagResult = target.apply(projFeatures, flagName); + hook(flagName[0], flagResult); + return flagResult; + }, + }; + const proxy = new Proxy(project.features.includes, handler); + project.features.includes = proxy; +} diff --git a/static/app/utils/featureObserver.spec.ts b/static/app/utils/featureObserver.spec.ts deleted file mode 100644 index d7722ff30d4479..00000000000000 --- a/static/app/utils/featureObserver.spec.ts +++ /dev/null @@ -1,251 +0,0 @@ -import {OrganizationFixture} from 'sentry-fixture/organization'; -import {ProjectFixture} from 'sentry-fixture/project'; - -import type {Organization} from 'sentry/types/organization'; -import type {Project} from 'sentry/types/project'; -import FeatureObserver from 'sentry/utils/featureObserver'; - -describe('FeatureObserver', () => { - let organization: Organization; - let project: Project; - - beforeEach(() => { - organization = OrganizationFixture({ - features: ['enable-issues', 'enable-profiling', 'enable-replay'], - }); - project = ProjectFixture({ - features: ['enable-proj-flag', 'enable-performance'], - }); - }); - - describe('observeOrganizationFlags', () => { - it('should add recently evaluated org flags to the flag queue', () => { - const inst = new FeatureObserver({bufferSize: 3}); - expect(organization.features).toEqual([ - 'enable-issues', - 'enable-profiling', - 'enable-replay', - ]); - - inst.observeOrganizationFlags({organization}); - expect(inst.getFeatureFlags().values).toEqual([]); - - organization.features.includes('enable-issues'); - organization.features.includes('replay-mobile-ui'); - expect(inst.getFeatureFlags().values).toEqual([ - {flag: 'feature.organizations:enable-issues', result: true}, - {flag: 'feature.organizations:replay-mobile-ui', result: false}, - ]); - - // do more evaluations to fill up and overflow the buffer - organization.features.includes('enable-replay'); - organization.features.includes('autofix-ui'); - organization.features.includes('new-issue-details'); - expect(inst.getFeatureFlags().values).toEqual([ - {flag: 'feature.organizations:enable-replay', result: true}, - {flag: 'feature.organizations:autofix-ui', result: false}, - {flag: 'feature.organizations:new-issue-details', result: false}, - ]); - }); - - it('should remove duplicate flags with a full queue', () => { - const inst = new FeatureObserver({bufferSize: 3}); - inst.observeOrganizationFlags({organization}); - expect(inst.getFeatureFlags().values).toEqual([]); - - organization.features.includes('enable-issues'); - organization.features.includes('replay-mobile-ui'); - organization.features.includes('enable-discover'); - expect(inst.getFeatureFlags().values).toEqual([ - {flag: 'feature.organizations:enable-issues', result: true}, - {flag: 'feature.organizations:replay-mobile-ui', result: false}, - {flag: 'feature.organizations:enable-discover', result: false}, - ]); - - // this is already in the queue; it should be removed and - // added back to the end of the queue - organization.features.includes('enable-issues'); - expect(inst.getFeatureFlags().values).toEqual([ - {flag: 'feature.organizations:replay-mobile-ui', result: false}, - {flag: 'feature.organizations:enable-discover', result: false}, - {flag: 'feature.organizations:enable-issues', result: true}, - ]); - - organization.features.includes('spam-ingest'); - expect(inst.getFeatureFlags().values).toEqual([ - {flag: 'feature.organizations:enable-discover', result: false}, - {flag: 'feature.organizations:enable-issues', result: true}, - {flag: 'feature.organizations:spam-ingest', result: false}, - ]); - - // this is already in the queue but in the back - // the queue should not change - organization.features.includes('spam-ingest'); - expect(inst.getFeatureFlags().values).toEqual([ - {flag: 'feature.organizations:enable-discover', result: false}, - {flag: 'feature.organizations:enable-issues', result: true}, - {flag: 'feature.organizations:spam-ingest', result: false}, - ]); - }); - - it('should remove duplicate flags with an unfilled queue', () => { - const inst = new FeatureObserver({bufferSize: 3}); - inst.observeOrganizationFlags({organization}); - expect(inst.getFeatureFlags().values).toEqual([]); - - organization.features.includes('enable-issues'); - organization.features.includes('replay-mobile-ui'); - expect(inst.getFeatureFlags().values).toEqual([ - {flag: 'feature.organizations:enable-issues', result: true}, - {flag: 'feature.organizations:replay-mobile-ui', result: false}, - ]); - - // this is already in the queue; it should be removed and - // added back to the end of the queue - organization.features.includes('enable-issues'); - expect(inst.getFeatureFlags().values).toEqual([ - {flag: 'feature.organizations:replay-mobile-ui', result: false}, - {flag: 'feature.organizations:enable-issues', result: true}, - ]); - - // this is already in the queue but in the back - // the queue should not change - organization.features.includes('enable-issues'); - expect(inst.getFeatureFlags().values).toEqual([ - {flag: 'feature.organizations:replay-mobile-ui', result: false}, - {flag: 'feature.organizations:enable-issues', result: true}, - ]); - }); - - it('should not change the functionality of `includes`', () => { - const inst = new FeatureObserver({bufferSize: 3}); - inst.observeOrganizationFlags({organization}); - expect(inst.getFeatureFlags().values).toEqual([]); - - organization.features.includes('enable-issues'); - organization.features.includes('replay-mobile-ui'); - expect(inst.getFeatureFlags().values).toEqual([ - {flag: 'feature.organizations:enable-issues', result: true}, - {flag: 'feature.organizations:replay-mobile-ui', result: false}, - ]); - - expect(organization.features.includes('enable-issues')).toBe(true); - expect(organization.features.includes('replay-mobile-ui')).toBe(false); - }); - }); - - describe('observeProjectFlags', () => { - it('should add recently evaluated proj flags to the flag queue', () => { - const inst = new FeatureObserver({bufferSize: 3}); - expect(project.features).toEqual(['enable-proj-flag', 'enable-performance']); - - inst.observeProjectFlags({project}); - expect(inst.getFeatureFlags().values).toEqual([]); - - project.features.includes('enable-proj-flag'); - project.features.includes('replay-mobile-ui'); - expect(inst.getFeatureFlags().values).toEqual([ - {flag: 'feature.projects:enable-proj-flag', result: true}, - {flag: 'feature.projects:replay-mobile-ui', result: false}, - ]); - - // do more evaluations to fill up and overflow the buffer - project.features.includes('enable-performance'); - project.features.includes('autofix-ui'); - project.features.includes('new-issue-details'); - expect(inst.getFeatureFlags().values).toEqual([ - {flag: 'feature.projects:enable-performance', result: true}, - {flag: 'feature.projects:autofix-ui', result: false}, - {flag: 'feature.projects:new-issue-details', result: false}, - ]); - }); - - it('should not change the functionality of `includes`', () => { - const inst = new FeatureObserver({bufferSize: 3}); - inst.observeProjectFlags({project}); - expect(inst.getFeatureFlags().values).toEqual([]); - - project.features.includes('enable-proj-flag'); - project.features.includes('replay-mobile-ui'); - expect(inst.getFeatureFlags().values).toEqual([ - {flag: 'feature.projects:enable-proj-flag', result: true}, - {flag: 'feature.projects:replay-mobile-ui', result: false}, - ]); - - expect(project.features.includes('enable-proj-flag')).toBe(true); - expect(project.features.includes('replay-mobile-ui')).toBe(false); - }); - }); - - describe('observeProjectFlags and observeOrganizationFlags', () => { - it('should add recently evaluated org and proj flags to the flag queue', () => { - const inst = new FeatureObserver({bufferSize: 3}); - expect(project.features).toEqual(['enable-proj-flag', 'enable-performance']); - expect(organization.features).toEqual([ - 'enable-issues', - 'enable-profiling', - 'enable-replay', - ]); - - inst.observeProjectFlags({project}); - inst.observeOrganizationFlags({organization}); - expect(inst.getFeatureFlags().values).toEqual([]); - - project.features.includes('enable-proj-flag'); - project.features.includes('enable-replay'); - organization.features.includes('enable-issues'); - expect(inst.getFeatureFlags().values).toEqual([ - {flag: 'feature.projects:enable-proj-flag', result: true}, - {flag: 'feature.projects:enable-replay', result: false}, - {flag: 'feature.organizations:enable-issues', result: true}, - ]); - - organization.features.includes('enable-replay'); - expect(inst.getFeatureFlags().values).toEqual([ - {flag: 'feature.projects:enable-replay', result: false}, - {flag: 'feature.organizations:enable-issues', result: true}, - {flag: 'feature.organizations:enable-replay', result: true}, - ]); - }); - - it('should keep the same queue if the project changes', () => { - const inst = new FeatureObserver({bufferSize: 3}); - expect(project.features).toEqual(['enable-proj-flag', 'enable-performance']); - expect(organization.features).toEqual([ - 'enable-issues', - 'enable-profiling', - 'enable-replay', - ]); - - inst.observeProjectFlags({project}); - inst.observeOrganizationFlags({organization}); - expect(inst.getFeatureFlags().values).toEqual([]); - - project.features.includes('enable-proj-flag'); - project.features.includes('enable-replay'); - organization.features.includes('enable-issues'); - expect(inst.getFeatureFlags().values).toEqual([ - {flag: 'feature.projects:enable-proj-flag', result: true}, - {flag: 'feature.projects:enable-replay', result: false}, - {flag: 'feature.organizations:enable-issues', result: true}, - ]); - - project = ProjectFixture({ - features: ['enable-new-flag'], - }); - inst.observeProjectFlags({project}); - expect(inst.getFeatureFlags().values).toEqual([ - {flag: 'feature.projects:enable-proj-flag', result: true}, - {flag: 'feature.projects:enable-replay', result: false}, - {flag: 'feature.organizations:enable-issues', result: true}, - ]); - - project.features.includes('enable-new-flag'); - expect(inst.getFeatureFlags().values).toEqual([ - {flag: 'feature.projects:enable-replay', result: false}, - {flag: 'feature.organizations:enable-issues', result: true}, - {flag: 'feature.projects:enable-new-flag', result: true}, - ]); - }); - }); -}); diff --git a/static/app/utils/featureObserver.ts b/static/app/utils/featureObserver.ts deleted file mode 100644 index 38df3ab8129e3b..00000000000000 --- a/static/app/utils/featureObserver.ts +++ /dev/null @@ -1,111 +0,0 @@ -import type {FeatureFlagContext} from '@sentry/core/build/types/types-hoist/context'; - -import type {Organization} from 'sentry/types/organization'; -import type {Project} from 'sentry/types/project'; - -const FEATURE_FLAG_BUFFER_SIZE = 100; -let __SINGLETON: FeatureObserver | null = null; - -export default class FeatureObserver { - /** - * Return the same instance of FeatureObserver in each part of the app. - * Multiple instances of FeatureObserver are needed by tests only. - */ - public static singleton({ - bufferSize = FEATURE_FLAG_BUFFER_SIZE, - }: { - bufferSize?: number; - }) { - if (!__SINGLETON) { - __SINGLETON = new FeatureObserver({bufferSize}); - } - return __SINGLETON; - } - - private _bufferSize = 0; - private FEATURE_FLAGS: FeatureFlagContext = {values: []}; - - constructor({bufferSize}: {bufferSize: number}) { - this._bufferSize = bufferSize; - } - - /** - * Return list of recently accessed feature flags. - */ - public getFeatureFlags() { - return this.FEATURE_FLAGS; - } - - public updateFlagBuffer({ - flagName, - flagResult, - }: { - flagName: string; - flagResult: boolean; - }) { - const flagBuffer = this.FEATURE_FLAGS; - // Check if the flag is already in the buffer - const index = flagBuffer.values.findIndex(f => f.flag === flagName); - - // The flag is already in the buffer - if (index !== -1) { - flagBuffer.values.splice(index, 1); - } - - // If at capacity, we need to remove the earliest flag - // This will only happen if not a duplicate flag - if (flagBuffer.values.length === this._bufferSize) { - flagBuffer.values.shift(); - } - - // Store the flag and its result in the buffer - flagBuffer.values.push({ - flag: flagName, - result: flagResult, - }); - } - - public observeOrganizationFlags({organization}: {organization: Organization}) { - // Track names of features that are passed into the .includes() function. - const handler = { - apply: (target: any, orgFeatures: string[], flagName: string[]) => { - // Evaluate the result of .includes() - const flagResult = target.apply(orgFeatures, flagName); - - // Append `feature.organizations:` in front to match the Sentry options automator format - const name = 'feature.organizations:' + flagName[0]; - - this.updateFlagBuffer({ - flagName: name, - flagResult, - }); - - return flagResult; - }, - }; - const proxy = new Proxy(organization.features.includes, handler); - organization.features.includes = proxy; - } - - public observeProjectFlags({project}: {project: Project}) { - // Track names of features that are passed into the .includes() function. - const handler = { - apply: (target: any, projFeatures: string[], flagName: string[]) => { - // Evaluate the result of .includes() - const flagResult = target.apply(projFeatures, flagName); - - // Append `feature.projects:` in front to match the Sentry options automator format - const name = 'feature.projects:' + flagName[0]; - - this.updateFlagBuffer({ - flagName: name, - flagResult, - }); - - return flagResult; - }, - }; - const proxy = new Proxy(project.features.includes, handler); - project.features.includes = proxy; - } -} diff --git a/static/app/views/projects/projectContext.tsx b/static/app/views/projects/projectContext.tsx index e7558d449a2fa3..fedaf37d1026f4 100644 --- a/static/app/views/projects/projectContext.tsx +++ b/static/app/views/projects/projectContext.tsx @@ -17,7 +17,7 @@ import {space} from 'sentry/styles/space'; import type {Organization} from 'sentry/types/organization'; import type {Project} from 'sentry/types/project'; import type {User} from 'sentry/types/user'; -import FeatureObserver from 'sentry/utils/featureObserver'; +import {addProjectFeaturesHook, getSentryFeaturesHook} from 'sentry/utils/featureFlags'; import withApi from 'sentry/utils/withApi'; import withOrganization from 'sentry/utils/withOrganization'; import withProjects from 'sentry/utils/withProjects'; @@ -181,8 +181,10 @@ class ProjectContextProvider extends Component { // assuming here that this means the project is considered the active project setActiveProject(project); - FeatureObserver.singleton({}).observeProjectFlags({ + const sentryFeaturesHook = getSentryFeaturesHook(); + addProjectFeaturesHook({ project, + hook: sentryFeaturesHook, }); } catch (error) { this.setState({ From 935a7d944e347d59ea0b17089049eb0954879324 Mon Sep 17 00:00:00 2001 From: Andrew Liu <159852527+aliu39@users.noreply.github.com> Date: Wed, 11 Dec 2024 10:51:28 -0800 Subject: [PATCH 2/4] Add hooks unit tests --- static/app/utils/featureFlags.spec.ts | 69 +++++++++++++++++++++++++++ static/app/utils/featureFlags.ts | 16 +++---- 2 files changed, 77 insertions(+), 8 deletions(-) create mode 100644 static/app/utils/featureFlags.spec.ts diff --git a/static/app/utils/featureFlags.spec.ts b/static/app/utils/featureFlags.spec.ts new file mode 100644 index 00000000000000..6d1982c1d779af --- /dev/null +++ b/static/app/utils/featureFlags.spec.ts @@ -0,0 +1,69 @@ +import {OrganizationFixture} from 'sentry-fixture/organization'; +import {ProjectFixture} from 'sentry-fixture/project'; + +import { + addOrganizationFeaturesHook, + addProjectFeaturesHook, +} from 'sentry/utils/featureFlags'; + +describe('addOrganizationFeaturesHook', () => { + let organization; + + beforeEach(() => { + organization = OrganizationFixture({ + features: ['enable-issues', 'enable-replay'], + }); + }); + + it('should pass the flag name and result to the hook on each evaluation', () => { + const mockHook = jest.fn(); + addOrganizationFeaturesHook({organization, hook: mockHook}); + + organization.features.includes('enable-replay'); + organization.features.includes('replay-mobile-ui'); + organization.features.includes('enable-issues'); + + expect(mockHook).toHaveBeenNthCalledWith(1, 'enable-replay', true); + expect(mockHook).toHaveBeenNthCalledWith(2, 'replay-mobile-ui', false); + expect(mockHook).toHaveBeenNthCalledWith(3, 'enable-issues', true); + }); + + it('should not change the functionality of `includes`', () => { + const mockHook = jest.fn(); + addOrganizationFeaturesHook({organization, hook: mockHook}); + expect(organization.features.includes('enable-issues')).toBe(true); + expect(organization.features.includes('enable-replay')).toBe(true); + expect(organization.features.includes('replay-mobile-ui')).toBe(false); + }); +}); + +describe('addProjectFeaturesHook', () => { + let project; + + beforeEach(() => { + project = ProjectFixture({ + features: ['enable-issues', 'enable-replay'], + }); + }); + + it('should pass the flag name and result to the hook on each evaluation', () => { + const mockHook = jest.fn(); + addProjectFeaturesHook({project, hook: mockHook}); + + project.features.includes('enable-replay'); + project.features.includes('replay-mobile-ui'); + project.features.includes('enable-issues'); + + expect(mockHook).toHaveBeenNthCalledWith(1, 'enable-replay', true); + expect(mockHook).toHaveBeenNthCalledWith(2, 'replay-mobile-ui', false); + expect(mockHook).toHaveBeenNthCalledWith(3, 'enable-issues', true); + }); + + it('should not change the functionality of `includes`', () => { + const mockHook = jest.fn(); + addProjectFeaturesHook({project, hook: mockHook}); + expect(project.features.includes('enable-issues')).toBe(true); + expect(project.features.includes('enable-replay')).toBe(true); + expect(project.features.includes('replay-mobile-ui')).toBe(false); + }); +}); diff --git a/static/app/utils/featureFlags.ts b/static/app/utils/featureFlags.ts index 10ccdb0f508a94..ccb1e93d8cb442 100644 --- a/static/app/utils/featureFlags.ts +++ b/static/app/utils/featureFlags.ts @@ -38,9 +38,9 @@ export function addOrganizationFeaturesHook({ organization: Organization; }) { const handler = { - apply: (target: any, orgFeatures: string[], flagName: string[]) => { - // Evaluate the result of .includes() - const flagResult = target.apply(orgFeatures, flagName); + apply: (includes: any, orgFeatures: string[], flagName: string[]) => { + // Evaluate the result of .includes() and pass it to hook before returning + const flagResult = includes.apply(orgFeatures, flagName); hook(flagName[0], flagResult); return flagResult; }, @@ -50,8 +50,8 @@ export function addOrganizationFeaturesHook({ } /** - * Registers a handler to track names and values of features passed into the - * project.features.includes() function. + * Registers a hook that processes feature names and values on each call to + * organization.features.includes(). */ export function addProjectFeaturesHook({ project, @@ -61,9 +61,9 @@ export function addProjectFeaturesHook({ project: Project; }) { const handler = { - apply: (target: any, projFeatures: string[], flagName: string[]) => { - // Evaluate the result of .includes() - const flagResult = target.apply(projFeatures, flagName); + apply: (includes: any, projFeatures: string[], flagName: string[]) => { + // Evaluate the result of .includes() and pass it to hook before returning + const flagResult = includes.apply(projFeatures, flagName); hook(flagName[0], flagResult); return flagResult; }, From 110494f1266ef6703a591631fba26241b4b05bef Mon Sep 17 00:00:00 2001 From: Andrew Liu <159852527+aliu39@users.noreply.github.com> Date: Wed, 11 Dec 2024 13:08:54 -0800 Subject: [PATCH 3/4] Use prefix param for getSentryFeaturesHook + inline call --- static/app/actionCreators/organization.tsx | 3 +-- static/app/utils/featureFlags.ts | 9 +++++++-- static/app/views/projects/projectContext.tsx | 3 +-- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/static/app/actionCreators/organization.tsx b/static/app/actionCreators/organization.tsx index 1ca0b216fc31e6..76cfc7943e9525 100644 --- a/static/app/actionCreators/organization.tsx +++ b/static/app/actionCreators/organization.tsx @@ -45,10 +45,9 @@ async function fetchOrg( } FeatureFlagOverrides.singleton().loadOrg(org); - const sentryFeaturesHook = getSentryFeaturesHook(); addOrganizationFeaturesHook({ organization: org, - hook: sentryFeaturesHook, + hook: getSentryFeaturesHook('feature.organizations:'), }); OrganizationStore.onUpdate(org, {replace: true}); diff --git a/static/app/utils/featureFlags.ts b/static/app/utils/featureFlags.ts index ccb1e93d8cb442..e802a096f1a622 100644 --- a/static/app/utils/featureFlags.ts +++ b/static/app/utils/featureFlags.ts @@ -8,8 +8,13 @@ import type {Project} from 'sentry/types/project'; * Returns a callback that can be used to track sentry flag evaluations through * the Sentry SDK, in the event context. If the FeatureFlagsIntegration is not * installed, the callback is a no-op. + * + * @param prefix - optionally specifies a prefix for flag names, before calling + * the SDK hook */ -export function getSentryFeaturesHook(): (name: string, value: unknown) => void { +export function getSentryFeaturesHook( + prefix?: string +): (name: string, value: unknown) => void { const featureFlagsIntegration = Sentry.getClient()?.getIntegrationByName( 'FeatureFlags' @@ -22,7 +27,7 @@ export function getSentryFeaturesHook(): (name: string, value: unknown) => void } return (name: string, value: unknown) => { // Append `feature.organizations:` in front to match the Sentry options automator format - featureFlagsIntegration?.addFeatureFlag('feature.organizations:' + name, value); + featureFlagsIntegration?.addFeatureFlag((prefix ?? '') + name, value); }; } diff --git a/static/app/views/projects/projectContext.tsx b/static/app/views/projects/projectContext.tsx index fedaf37d1026f4..040078f21452f2 100644 --- a/static/app/views/projects/projectContext.tsx +++ b/static/app/views/projects/projectContext.tsx @@ -181,10 +181,9 @@ class ProjectContextProvider extends Component { // assuming here that this means the project is considered the active project setActiveProject(project); - const sentryFeaturesHook = getSentryFeaturesHook(); addProjectFeaturesHook({ project, - hook: sentryFeaturesHook, + hook: getSentryFeaturesHook('feature.projects:'), }); } catch (error) { this.setState({ From 9ad0a32e66c700b196fe3a555f24565c52506a83 Mon Sep 17 00:00:00 2001 From: Andrew Liu <159852527+aliu39@users.noreply.github.com> Date: Mon, 16 Dec 2024 10:39:56 -0800 Subject: [PATCH 4/4] Rename hook->handler, getSentryFeaturesHook -> buildSentryFeaturesHandler --- static/app/actionCreators/organization.tsx | 8 ++-- static/app/utils/featureFlags.spec.ts | 40 ++++++++++---------- static/app/utils/featureFlags.ts | 30 +++++++-------- static/app/views/projects/projectContext.tsx | 9 +++-- 4 files changed, 45 insertions(+), 42 deletions(-) diff --git a/static/app/actionCreators/organization.tsx b/static/app/actionCreators/organization.tsx index 76cfc7943e9525..f8dd736234049e 100644 --- a/static/app/actionCreators/organization.tsx +++ b/static/app/actionCreators/organization.tsx @@ -16,8 +16,8 @@ import type {Organization, Team} from 'sentry/types/organization'; import type {Project} from 'sentry/types/project'; import FeatureFlagOverrides from 'sentry/utils/featureFlagOverrides'; import { - addOrganizationFeaturesHook, - getSentryFeaturesHook, + addOrganizationFeaturesHandler, + buildSentryFeaturesHandler, } from 'sentry/utils/featureFlags'; import {getPreloadedDataPromise} from 'sentry/utils/getPreloadedData'; import parseLinkHeader from 'sentry/utils/parseLinkHeader'; @@ -45,9 +45,9 @@ async function fetchOrg( } FeatureFlagOverrides.singleton().loadOrg(org); - addOrganizationFeaturesHook({ + addOrganizationFeaturesHandler({ organization: org, - hook: getSentryFeaturesHook('feature.organizations:'), + handler: buildSentryFeaturesHandler('feature.organizations:'), }); OrganizationStore.onUpdate(org, {replace: true}); diff --git a/static/app/utils/featureFlags.spec.ts b/static/app/utils/featureFlags.spec.ts index 6d1982c1d779af..fa6e9a983fb37d 100644 --- a/static/app/utils/featureFlags.spec.ts +++ b/static/app/utils/featureFlags.spec.ts @@ -2,11 +2,11 @@ import {OrganizationFixture} from 'sentry-fixture/organization'; import {ProjectFixture} from 'sentry-fixture/project'; import { - addOrganizationFeaturesHook, - addProjectFeaturesHook, + addOrganizationFeaturesHandler, + addProjectFeaturesHandler, } from 'sentry/utils/featureFlags'; -describe('addOrganizationFeaturesHook', () => { +describe('addOrganizationFeaturesHandler', () => { let organization; beforeEach(() => { @@ -15,29 +15,29 @@ describe('addOrganizationFeaturesHook', () => { }); }); - it('should pass the flag name and result to the hook on each evaluation', () => { - const mockHook = jest.fn(); - addOrganizationFeaturesHook({organization, hook: mockHook}); + it('should pass the flag name and result to the handler on each evaluation', () => { + const mockHandler = jest.fn(); + addOrganizationFeaturesHandler({organization, handler: mockHandler}); organization.features.includes('enable-replay'); organization.features.includes('replay-mobile-ui'); organization.features.includes('enable-issues'); - expect(mockHook).toHaveBeenNthCalledWith(1, 'enable-replay', true); - expect(mockHook).toHaveBeenNthCalledWith(2, 'replay-mobile-ui', false); - expect(mockHook).toHaveBeenNthCalledWith(3, 'enable-issues', true); + expect(mockHandler).toHaveBeenNthCalledWith(1, 'enable-replay', true); + expect(mockHandler).toHaveBeenNthCalledWith(2, 'replay-mobile-ui', false); + expect(mockHandler).toHaveBeenNthCalledWith(3, 'enable-issues', true); }); it('should not change the functionality of `includes`', () => { - const mockHook = jest.fn(); - addOrganizationFeaturesHook({organization, hook: mockHook}); + const mockHandler = jest.fn(); + addOrganizationFeaturesHandler({organization, handler: mockHandler}); expect(organization.features.includes('enable-issues')).toBe(true); expect(organization.features.includes('enable-replay')).toBe(true); expect(organization.features.includes('replay-mobile-ui')).toBe(false); }); }); -describe('addProjectFeaturesHook', () => { +describe('addProjectFeaturesHandler', () => { let project; beforeEach(() => { @@ -46,22 +46,22 @@ describe('addProjectFeaturesHook', () => { }); }); - it('should pass the flag name and result to the hook on each evaluation', () => { - const mockHook = jest.fn(); - addProjectFeaturesHook({project, hook: mockHook}); + it('should pass the flag name and result to the handler on each evaluation', () => { + const mockHandler = jest.fn(); + addProjectFeaturesHandler({project, handler: mockHandler}); project.features.includes('enable-replay'); project.features.includes('replay-mobile-ui'); project.features.includes('enable-issues'); - expect(mockHook).toHaveBeenNthCalledWith(1, 'enable-replay', true); - expect(mockHook).toHaveBeenNthCalledWith(2, 'replay-mobile-ui', false); - expect(mockHook).toHaveBeenNthCalledWith(3, 'enable-issues', true); + expect(mockHandler).toHaveBeenNthCalledWith(1, 'enable-replay', true); + expect(mockHandler).toHaveBeenNthCalledWith(2, 'replay-mobile-ui', false); + expect(mockHandler).toHaveBeenNthCalledWith(3, 'enable-issues', true); }); it('should not change the functionality of `includes`', () => { - const mockHook = jest.fn(); - addProjectFeaturesHook({project, hook: mockHook}); + const mockHandler = jest.fn(); + addProjectFeaturesHandler({project, handler: mockHandler}); expect(project.features.includes('enable-issues')).toBe(true); expect(project.features.includes('enable-replay')).toBe(true); expect(project.features.includes('replay-mobile-ui')).toBe(false); diff --git a/static/app/utils/featureFlags.ts b/static/app/utils/featureFlags.ts index e802a096f1a622..57384f81ebc8de 100644 --- a/static/app/utils/featureFlags.ts +++ b/static/app/utils/featureFlags.ts @@ -12,7 +12,7 @@ import type {Project} from 'sentry/types/project'; * @param prefix - optionally specifies a prefix for flag names, before calling * the SDK hook */ -export function getSentryFeaturesHook( +export function buildSentryFeaturesHandler( prefix?: string ): (name: string, value: unknown) => void { const featureFlagsIntegration = @@ -32,47 +32,47 @@ export function getSentryFeaturesHook( } /** - * Registers a hook that processes feature names and values on each call to + * Registers a handler that processes feature names and values on each call to * organization.features.includes(). */ -export function addOrganizationFeaturesHook({ +export function addOrganizationFeaturesHandler({ organization, - hook, + handler, }: { - hook: (name: string, value: unknown) => void; + handler: (name: string, value: unknown) => void; organization: Organization; }) { - const handler = { + const includesHandler = { apply: (includes: any, orgFeatures: string[], flagName: string[]) => { // Evaluate the result of .includes() and pass it to hook before returning const flagResult = includes.apply(orgFeatures, flagName); - hook(flagName[0], flagResult); + handler(flagName[0], flagResult); return flagResult; }, }; - const proxy = new Proxy(organization.features.includes, handler); + const proxy = new Proxy(organization.features.includes, includesHandler); organization.features.includes = proxy; } /** - * Registers a hook that processes feature names and values on each call to + * Registers a handler that processes feature names and values on each call to * organization.features.includes(). */ -export function addProjectFeaturesHook({ +export function addProjectFeaturesHandler({ project, - hook, + handler, }: { - hook: (name: string, value: unknown) => void; + handler: (name: string, value: unknown) => void; project: Project; }) { - const handler = { + const includesHandler = { apply: (includes: any, projFeatures: string[], flagName: string[]) => { // Evaluate the result of .includes() and pass it to hook before returning const flagResult = includes.apply(projFeatures, flagName); - hook(flagName[0], flagResult); + handler(flagName[0], flagResult); return flagResult; }, }; - const proxy = new Proxy(project.features.includes, handler); + const proxy = new Proxy(project.features.includes, includesHandler); project.features.includes = proxy; } diff --git a/static/app/views/projects/projectContext.tsx b/static/app/views/projects/projectContext.tsx index 040078f21452f2..b03f4e8d58cf10 100644 --- a/static/app/views/projects/projectContext.tsx +++ b/static/app/views/projects/projectContext.tsx @@ -17,7 +17,10 @@ import {space} from 'sentry/styles/space'; import type {Organization} from 'sentry/types/organization'; import type {Project} from 'sentry/types/project'; import type {User} from 'sentry/types/user'; -import {addProjectFeaturesHook, getSentryFeaturesHook} from 'sentry/utils/featureFlags'; +import { + addProjectFeaturesHandler, + buildSentryFeaturesHandler, +} from 'sentry/utils/featureFlags'; import withApi from 'sentry/utils/withApi'; import withOrganization from 'sentry/utils/withOrganization'; import withProjects from 'sentry/utils/withProjects'; @@ -181,9 +184,9 @@ class ProjectContextProvider extends Component { // assuming here that this means the project is considered the active project setActiveProject(project); - addProjectFeaturesHook({ + addProjectFeaturesHandler({ project, - hook: getSentryFeaturesHook('feature.projects:'), + handler: buildSentryFeaturesHandler('feature.projects:'), }); } catch (error) { this.setState({