Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
…ailable before registration
There was a problem hiding this comment.
Do you need onPluginsReady in the base plugin? I think this was added to solve the "two plugins need each other" interaction back when Agustine was working on Replay and Observability being separate plugins but needing to wait until both registered. Spec requirement
My impression is it makes sense to add while you're working on this story.
There was a problem hiding this comment.
I needed that logic but I did workaround in Obs-SDK, I count number of getHooks calls and number of register calls. When they are equal it means everything is ready
|
|
||
| var applicationMetadata = new ApplicationMetadata( | ||
| applicationInfo.ApplicationId, | ||
| applicationInfo.ApplicationVersion |
There was a problem hiding this comment.
There is also Name and VersionName params that should be carried over.
There was a problem hiding this comment.
Do you need to add optional fields id and version in PluginMetadata? https://github.com/launchdarkly/sdk-specs/tree/main/specs/PLUGIN-sdk-plugin-support#requirement-113
Also related to the same sort of problem Agustine was solving.
| } | ||
|
|
||
| _hookExecutor = _pluginHooks.Any() | ||
| ? (IHookExecutor)new Executor(_log.SubLogger(LogNames.HooksSubLog), _pluginHooks) |
There was a problem hiding this comment.
_pluginHooks is only used here. Does it need to be a private member on LDClient? Could it just be local in this function that populates it.
|
|
||
| PluginConfiguration pluginConfig = null; | ||
| EnvironmentMetadata environmentMetadata = null; | ||
| if (_config.Plugins != null) |
There was a problem hiding this comment.
The nullity, null checks, assignment, and usage of nullables in these next 20 lines of code seems fragile. Consider structuring differently so that their interaction is easier to discern.
|
|
||
| PluginConfiguration pluginConfig = null; | ||
| EnvironmentMetadata environmentMetadata = null; | ||
| if (_config.Plugins != null) |
There was a problem hiding this comment.
I think the server SDK uses Components.Plugins() if no config is provided. This may be an option to deal with the missing config in an elegant way.
| /// <param name="context">parameters associated with this identify operation</param> | ||
| /// <param name="data">user-configurable data, currently empty</param> | ||
| /// <returns>user-configurable data, which will be forwarded to <see cref="AfterIdentify"/></returns> | ||
| public virtual SeriesData BeforeIdentify(IdentifySeriesContext context, SeriesData data) => |
There was a problem hiding this comment.
I don't see BeforeIdentify and AfterIdentify being invoked anywhere. Is that in scope of this PR? Seems like it must be if the Hook is now available. Also need tests for this case.
There was a problem hiding this comment.
Ryan said that Evaluation flag is must to be PR mergable
| bool checkType, EventFactory eventFactory) | ||
| { | ||
| var evalSeriesContext = new EvaluationSeriesContext(featureKey, Context, defaultJson, | ||
| GetMethodName<T>(eventFactory)); |
There was a problem hiding this comment.
Is environment id ever passed in to evaluation series context? I can't recall if Dotnet Client got support for this from the data source (ultimately from the headers on the responses from the server).
Summary
This PR adds plugin architecture and hook-based flag evaluation lifecycle support to the LaunchDarkly Client SDK for .NET, ported from the Server SDK implementation.
Plugin System
Pluginbase class (Plugins/Plugin.cs) that provides aGetHooks()method, allowing plugins to register hooks with the SDKPluginConfigurationandPluginConfigurationBuilderto configure plugins viaConfigurationBuilder.Plugins()LdClientto initialize plugins during startup, collect their hooks, and dispose of them properly during shutdownLdClientcleanup logic to ensure proper disposal order of resourcesHook Framework
Hookbase class with virtualBeforeEvaluation,AfterEvaluation,BeforeIdentify, andAfterIdentifystage methods, plusIDisposablesupportEvaluationSeriesContext- captures flag key, context, default value, method, and environment ID for evaluation hooksIdentifySeriesContext/IdentifySeriesResult- context and result types for identify hooksMethodconstants mapping to SDK variation methods (e.g.LdClient.BoolVariation)SeriesDataBuilder- fluent API for building immutable series data passed between hook stagesHook Execution Engine
Executor/NoopExecutorimplementingIHookExecutor- manages hook lifecycle with proper forward/reverse orderingBeforeEvaluation/AfterEvaluationstage executors with error isolation - exceptions in one hook do not prevent other hooks from executingLdClient.EvaluateInternal(), wrapping flag evaluations withBeforeEvaluationandAfterEvaluationcallsGetMethodName<T>()to resolve the correctMethodconstant based on variation type and detail modeEvaluateInternaldirectly, avoiding recursive hook invocationsTest Plan
EvaluationSeriesTest- hooks execute in LIFO order (forward before, reverse after)EvaluationSeriesTest- exceptions in hook stages don't prevent other hooks from runningEvaluationSeriesTest- stage failures log expected error messages with flag key and hook nameEvaluationSeriesTest- disposing executor disposes all registered hooksPluginConfigurationBuilderTest- plugin configuration builder tests passLdClientPluginTests- end-to-end plugin/hook integration tests passdotnet test --filter EvaluationSeriesTest)Note
Medium Risk
Touches core
LdClientevaluation flow by wrapping allVariation*calls with hook execution and adds plugin registration at startup, so regressions could affect flag evaluation behavior/performance despite safeguards like noop execution when unconfigured.Overview
Adds client-side plugin support by introducing
PluginConfigurationBuilder/PluginConfigurationand wiring a newPlugins(...)option throughComponents,ConfigurationBuilder, andConfiguration.Integrates plugin lifecycle into
LdClientinitialization: builds plugin config, createsEnvironmentMetadata, collects plugin-providedHooks, instantiates a hookExecutor(orNoopExecutorwhen none), then registers plugins; hook execution is disposed with the client and logs under a newLogNames.HooksSubLog.Introduces a hook API and execution pipeline for evaluations (
Hook,EvaluationSeriesContext, stage executors) and wrapsVariationInternalso hooks run before/after evaluations; prerequisite evaluations are refactored to callEvaluateInternaldirectly to avoid triggering hooks. Adds unit tests covering hook ordering/error isolation/disposal and plugin builder/registration behavior.Written by Cursor Bugbot for commit e5d9dc0. This will update automatically on new commits. Configure here.