-
Notifications
You must be signed in to change notification settings - Fork 31
feat: allow clients to evaluate bootstrapped flags when not ready #1036
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
Changes from 4 commits
0b9ed16
e1d1079
158db3f
2d45b52
b7b4bf8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,10 @@ import { LDIdentifyOptions } from './api/LDIdentifyOptions'; | |
| import { createAsyncTaskQueue } from './async/AsyncTaskQueue'; | ||
| import { Configuration, ConfigurationImpl, LDClientInternalOptions } from './configuration'; | ||
| import { addAutoEnv } from './context/addAutoEnv'; | ||
| import { | ||
| ActiveContextTracker, | ||
| createActiveContextTracker, | ||
| } from './context/createActiveContextTracker'; | ||
| import { ensureKey } from './context/ensureKey'; | ||
| import { DataManager, DataManagerFactory } from './DataManager'; | ||
| import createDiagnosticsManager from './diagnostics/createDiagnosticsManager'; | ||
|
|
@@ -43,6 +47,7 @@ import createEventProcessor from './events/createEventProcessor'; | |
| import EventFactory from './events/EventFactory'; | ||
| import DefaultFlagManager, { FlagManager } from './flag-manager/FlagManager'; | ||
| import { FlagChangeType } from './flag-manager/FlagUpdater'; | ||
| import { ItemDescriptor } from './flag-manager/ItemDescriptor'; | ||
| import HookRunner from './HookRunner'; | ||
| import { getInspectorHook } from './inspection/getInspectorHook'; | ||
| import InspectorManager from './inspection/InspectorManager'; | ||
|
|
@@ -55,12 +60,12 @@ const DEFAULT_IDENTIFY_TIMEOUT_SECONDS = 5; | |
|
|
||
| export default class LDClientImpl implements LDClient, LDClientIdentifyResult { | ||
| private readonly _config: Configuration; | ||
| private _uncheckedContext?: LDContext; | ||
| private _checkedContext?: Context; | ||
| private readonly _diagnosticsManager?: internal.DiagnosticsManager; | ||
| private _eventProcessor?: internal.EventProcessor; | ||
| readonly logger: LDLogger; | ||
|
|
||
| private _activeContextTracker: ActiveContextTracker = createActiveContextTracker(); | ||
|
|
||
| private readonly _highTimeoutThreshold: number = 15; | ||
|
|
||
| private _eventFactoryDefault = new EventFactory(false); | ||
|
|
@@ -200,27 +205,22 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult { | |
| // code. We are returned the unchecked context so that if a consumer identifies with an invalid context | ||
| // and then calls getContext, they get back the same context they provided, without any assertion about | ||
| // validity. | ||
| return this._uncheckedContext ? clone<LDContext>(this._uncheckedContext) : undefined; | ||
| return this._activeContextTracker.hasContext() | ||
| ? clone<LDContext>(this._activeContextTracker.getPristineContext()) | ||
| : undefined; | ||
| } | ||
|
|
||
| protected getInternalContext(): Context | undefined { | ||
| return this._checkedContext; | ||
| return this._activeContextTracker.getContext(); | ||
| } | ||
|
|
||
| private _createIdentifyPromise(): { | ||
| identifyPromise: Promise<void>; | ||
| identifyResolve: () => void; | ||
| identifyReject: (err: Error) => void; | ||
| } { | ||
| let res: any; | ||
| let rej: any; | ||
|
|
||
| const basePromise = new Promise<void>((resolve, reject) => { | ||
| res = resolve; | ||
| rej = reject; | ||
| }); | ||
|
|
||
| return { identifyPromise: basePromise, identifyResolve: res, identifyReject: rej }; | ||
| /** | ||
| * Preset flags are used to set the flags before the client is initialized. This is useful for | ||
| * when client has precached flags that are ready to evaluate without full initialization. | ||
| * @param newFlags - The flags to preset. | ||
| */ | ||
| protected presetFlags(newFlags: { [key: string]: ItemDescriptor }) { | ||
| this._flagManager.presetFlags(newFlags); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -307,15 +307,14 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult { | |
| this.emitter.emit('error', context, error); | ||
| return Promise.reject(error); | ||
| } | ||
| this._uncheckedContext = context; | ||
| this._checkedContext = checkedContext; | ||
| this._activeContextTracker.set(context, checkedContext); | ||
|
|
||
| this._eventProcessor?.sendEvent( | ||
| this._eventFactoryDefault.identifyEvent(this._checkedContext), | ||
| this._eventFactoryDefault.identifyEvent(checkedContext), | ||
| ); | ||
| const { identifyPromise, identifyResolve, identifyReject } = | ||
| this._createIdentifyPromise(); | ||
| this.logger.debug(`Identifying ${JSON.stringify(this._checkedContext)}`); | ||
| this._activeContextTracker.newIdentificationPromise(); | ||
| this.logger.debug(`Identifying ${JSON.stringify(checkedContext)}`); | ||
|
|
||
| await this.dataManager.identify( | ||
| identifyResolve, | ||
|
|
@@ -370,7 +369,7 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult { | |
| } | ||
|
|
||
| track(key: string, data?: any, metricValue?: number): void { | ||
| if (!this._checkedContext || !this._checkedContext.valid) { | ||
| if (!this._activeContextTracker.hasValidContext()) { | ||
| this.logger.warn(ClientMessages.MissingContextKeyNoEvent); | ||
| return; | ||
| } | ||
|
|
@@ -382,14 +381,19 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult { | |
|
|
||
| this._eventProcessor?.sendEvent( | ||
| this._config.trackEventModifier( | ||
| this._eventFactoryDefault.customEvent(key, this._checkedContext!, data, metricValue), | ||
| this._eventFactoryDefault.customEvent( | ||
| key, | ||
| this._activeContextTracker.getContext()!, | ||
| data, | ||
| metricValue, | ||
| ), | ||
| ), | ||
| ); | ||
|
|
||
| this._hookRunner.afterTrack({ | ||
| key, | ||
| // The context is pre-checked above, so we know it can be unwrapped. | ||
| context: this._uncheckedContext!, | ||
| context: this._activeContextTracker.getPristineContext()!, | ||
| data, | ||
| metricValue, | ||
| }); | ||
|
|
@@ -401,23 +405,34 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult { | |
| eventFactory: EventFactory, | ||
| typeChecker?: (value: any) => [boolean, string], | ||
| ): LDEvaluationDetail { | ||
| if (!this._uncheckedContext) { | ||
| this.logger.debug(ClientMessages.MissingContextKeyNoEvent); | ||
| return createErrorEvaluationDetail(ErrorKinds.UserNotSpecified, defaultValue); | ||
| // We are letting evaulations happen without a context. The main case for this | ||
| // is when cached data is loaded, but the client is not fully initialized. In this | ||
| // case, we will write out a warning for each evaluation attempt. | ||
|
|
||
| // NOTE: we will be changing this behavior soon once we have a tracker on the | ||
| // client initialization state. | ||
| const hasContext = this._activeContextTracker.hasContext(); | ||
| if (!hasContext) { | ||
| this.logger?.warn( | ||
| 'Flag evaluation called before client is fully initialized, data from this evaulation could be stale.', | ||
| ); | ||
| } | ||
|
|
||
| const evalContext = Context.fromLDContext(this._uncheckedContext); | ||
| const evalContext = this._activeContextTracker.getContext()!; | ||
| const foundItem = this._flagManager.get(flagKey); | ||
|
|
||
| if (foundItem === undefined || foundItem.flag.deleted) { | ||
| const defVal = defaultValue ?? null; | ||
| const error = new LDClientError( | ||
| `Unknown feature flag "${flagKey}"; returning default value ${defVal}.`, | ||
| ); | ||
| this.emitter.emit('error', this._uncheckedContext, error); | ||
| this._eventProcessor?.sendEvent( | ||
| this._eventFactoryDefault.unknownFlagEvent(flagKey, defVal, evalContext), | ||
| ); | ||
|
|
||
| this.emitter.emit('error', this._activeContextTracker.getPristineContext(), error); | ||
|
||
| if (hasContext) { | ||
| this._eventProcessor?.sendEvent( | ||
| this._eventFactoryDefault.unknownFlagEvent(flagKey, defVal, evalContext), | ||
| ); | ||
| } | ||
| return createErrorEvaluationDetail(ErrorKinds.FlagNotFound, defaultValue); | ||
| } | ||
|
|
||
|
|
@@ -426,20 +441,22 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult { | |
| if (typeChecker) { | ||
| const [matched, type] = typeChecker(value); | ||
| if (!matched) { | ||
| this._eventProcessor?.sendEvent( | ||
| eventFactory.evalEventClient( | ||
| flagKey, | ||
| defaultValue, // track default value on type errors | ||
| defaultValue, | ||
| foundItem.flag, | ||
| evalContext, | ||
| reason, | ||
| ), | ||
| ); | ||
| if (hasContext) { | ||
| this._eventProcessor?.sendEvent( | ||
| eventFactory.evalEventClient( | ||
| flagKey, | ||
| defaultValue, // track default value on type errors | ||
| defaultValue, | ||
| foundItem.flag, | ||
| evalContext, | ||
| reason, | ||
| ), | ||
| ); | ||
| } | ||
| const error = new LDClientError( | ||
| `Wrong type "${type}" for feature flag "${flagKey}"; returning default value`, | ||
| ); | ||
| this.emitter.emit('error', this._uncheckedContext, error); | ||
| this.emitter.emit('error', this._activeContextTracker.getPristineContext(), error); | ||
| return createErrorEvaluationDetail(ErrorKinds.WrongType, defaultValue); | ||
| } | ||
| } | ||
|
|
@@ -453,31 +470,36 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult { | |
| prerequisites?.forEach((prereqKey) => { | ||
| this._variationInternal(prereqKey, undefined, this._eventFactoryDefault); | ||
| }); | ||
| this._eventProcessor?.sendEvent( | ||
| eventFactory.evalEventClient( | ||
| flagKey, | ||
| value, | ||
| defaultValue, | ||
| foundItem.flag, | ||
| evalContext, | ||
| reason, | ||
| ), | ||
| ); | ||
| if (hasContext) { | ||
| this._eventProcessor?.sendEvent( | ||
| eventFactory.evalEventClient( | ||
| flagKey, | ||
| value, | ||
| defaultValue, | ||
| foundItem.flag, | ||
| evalContext, | ||
| reason, | ||
| ), | ||
| ); | ||
| } | ||
| return successDetail; | ||
| } | ||
|
|
||
| variation(flagKey: string, defaultValue?: LDFlagValue): LDFlagValue { | ||
| const { value } = this._hookRunner.withEvaluation( | ||
| flagKey, | ||
| this._uncheckedContext, | ||
| this._activeContextTracker.getPristineContext(), | ||
| defaultValue, | ||
| () => this._variationInternal(flagKey, defaultValue, this._eventFactoryDefault), | ||
| ); | ||
| return value; | ||
| } | ||
| variationDetail(flagKey: string, defaultValue?: LDFlagValue): LDEvaluationDetail { | ||
| return this._hookRunner.withEvaluation(flagKey, this._uncheckedContext, defaultValue, () => | ||
| this._variationInternal(flagKey, defaultValue, this._eventFactoryWithReasons), | ||
| return this._hookRunner.withEvaluation( | ||
| flagKey, | ||
| this._activeContextTracker.getPristineContext(), | ||
| defaultValue, | ||
| () => this._variationInternal(flagKey, defaultValue, this._eventFactoryWithReasons), | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -487,8 +509,11 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult { | |
| eventFactory: EventFactory, | ||
| typeChecker: (value: unknown) => [boolean, string], | ||
| ): LDEvaluationDetailTyped<T> { | ||
| return this._hookRunner.withEvaluation(key, this._uncheckedContext, defaultValue, () => | ||
| this._variationInternal(key, defaultValue, eventFactory, typeChecker), | ||
| return this._hookRunner.withEvaluation( | ||
| key, | ||
| this._activeContextTracker.getPristineContext(), | ||
| defaultValue, | ||
| () => this._variationInternal(key, defaultValue, eventFactory, typeChecker), | ||
| ); | ||
| } | ||
|
|
||
|
|
||
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: Bootstrap data parsed twice causing duplicate log warnings
When
identifyis called with bootstrap data,readFlagsFromBootstrapis invoked twice with the same data - first inBrowserClientImpl.identifyResultto callpresetFlags, then again inBrowserDataManager._finishIdentifyFromBootstrapto callsetBootstrap. This causes any warning logs (e.g., "bootstrap data did not include flag metadata") to appear twice, which could confuse users, and unnecessarily parses the same data twice. The bootstrap data could be parsed once and the result reused for both operations.