feat: Add environment ID support for hooks - #202
Conversation
Co-Authored-By: rlamb@launchdarkly.com <4955475+kinyoklion@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
@cursor review |
| * | ||
| * @param environmentId the environment ID reported by LaunchDarkly | ||
| */ | ||
| default void setEnvironmentId(String environmentId) {} |
There was a problem hiding this comment.
This is an interesting different approach. Because it changes the interface the data sources use instead of changing the interfaces of the sources themselves.
I do see that a downside could be that it makes it a 2 step process instead of a single step.
I also don't like that the environment ID doesn't live alongside the store.
There was a problem hiding this comment.
Agreed on both counts — I went this way because FDv1 has no metadata channel at all (DataStore.init(FullDataSet) carries only data + shouldPersist), so the sink was the one place both generations already meet.
The store-adjacent alternative, mirroring how the selector is handled: TransactionalDataStore already receives the whole change set, so it could expose getEnvironmentId() next to getSelector(), InMemoryDataStore retains what applyFullPayload is already handed, and FDv2DataSystem reads it from the store instead of the sink — one step, and it lives with the data. FDv1 would still need a channel; the least invasive is putting it on FullDataSet alongside shouldPersist so init stays single-step, with FDv1DataSystem reading it back off the store as well.
That's a bigger change to the store interfaces than what's here. Want me to rework it that way?
There was a problem hiding this comment.
I am going to leave it as is until there are more opinions. This code will be deprecated when we remove FDv1 and the FDv2 path should be cleaner.
There was a problem hiding this comment.
Sounds good — leaving it as is.
There was a problem hiding this comment.
I prefer approaches where it goes through to the store with the flag data and perfer one operation.
There was a problem hiding this comment.
That's the shape I sketched above: the env ID rides the change set into the store, TransactionalDataStore exposes getEnvironmentId() alongside getSelector(), and the data systems read it back from the store. For FDv1 it would go on FullDataSet so init stays one operation.
Happy to rework it that way — just say the word and I'll push it.
There was a problem hiding this comment.
Reworked in 2ab6efb: DataSourceUpdateSink.setEnvironmentId is gone. The env ID now rides the data — FullDataSet carries it for FDv1 (DefaultFeatureRequestor from the poll response, StreamProcessor from the put event's stream headers), FDv2 already had it on the change set — and the stores retain it: InMemoryDataStore/WriteThroughStore/PersistentDataStoreWrapper implement DataStore.getEnvironmentId(), which the data systems read for the hook context. Empty/absent values don't clear a retained ID.
Co-Authored-By: rlamb@launchdarkly.com <4955475+kinyoklion@users.noreply.github.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…re init Co-Authored-By: rlamb@launchdarkly.com <4955475+kinyoklion@users.noreply.github.com>
…uation EvaluatorWithHooks read the environment ID supplier outside of the exception guards that protect every other store access during evaluation, so a DataStore whose getEnvironmentId() throws would make the variation methods throw. Treat a failure as an unknown environment ID and log it, matching how hook stage failures are handled.
- LDClient-level tests for FDv1 streaming/polling and FDv2 streaming, polling synchronizer and polling initializer, plus the custom-DataStore, throwing-DataStore and failed-persistent-init cases. - PersistentDataStoreWrapper: ID retained on successful init in all cache modes, not retained when the underlying store fails with a finite cache, retained when caching indefinitely. - WriteThroughStore: null before the first initializing payload, exposed and forwarded to a legacy persistent store, not written in READ_ONLY mode, consistent with memory data when the persistent write fails. - InMemoryDataStore partial change sets and exportAll(); FDv1 adapter retention across partial change sets; PersistentDataStoreConverter; FullDataSet equality over shouldPersist and environmentId. - PollingProcessorTest: replace the sleep-based retention test with one that puts a different ID on error responses and synchronizes on data source status, so it fails if an ID is ever taken from an unsuccessful response. - DataSourceUpdatesImplTest: the legacy store double records the ID verbatim instead of re-implementing the store's retention logic.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 877bc2f. Configure here.
…mmary Avoids echoing potentially sensitive detail from a customer DataStore's exception into the SDK log, matching the convention used for other store-originated exceptions.
2 participants
Requirements
Related issues
Companion to the equivalent work in the other SDKs: launchdarkly/python-server-sdk#484, launchdarkly/cpp-sdks#594, launchdarkly/ruby-server-sdk#414.
Describe the solution you've provided
EvaluationSeriesContextgains anenvironmentIdfield, populated from theX-LD-EnvIDresponse header LaunchDarkly sends on flag delivery responses.FullDataSetcarries it for FDv1, and FDv2 change sets already did.DataStore.getEnvironmentId()(defaultnull) is implemented byInMemoryDataStore,WriteThroughStoreandPersistentDataStoreWrapper; the data systems read it from the store for the hook context.DefaultFeatureRequestor, after the response is confirmed successful), FDv1 streaming (StreamProcessor, from theputevent's stream headers), FDv2 initializers/synchronizers, and the FDv1 fallback adapter used under FDv2.environmentIdand declareshook-environment-id.Backend-only change, so no screenshots or staging preview apply.
Describe alternatives you've considered
A
DataSourceUpdateSink.setEnvironmentIdcall (the previous revision of this PR) — reviewers preferred a single operation that keeps the environment ID next to the data in the store.Additional context
Implementation details
EvaluatorWithHooksreceives a supplier so each evaluation sees the current value:FullDataSetgains a third constructor parameter (environmentId); the existing constructors delegate to it, so this is source and binary compatible for existing callers. Conversions that rebuild aFullDataSet(dependency sorting, persistent store serialization, change-set-to-legacy-init) now preserve the value.How to test
./gradlew test checkstyleMain checkstyleTest javadocinlib/sdk/serverhooks/evaluation/provides the environment IDpasses against released harness v2.39.0 (FDv1, default and polling) and v3.2.0-alpha.6 (FDv2); full suites also pass.Risks / follow-ups
v3.0.0-alpha.6, so CI won't exercise the new test until that pin is bumped.DataStoreimplementations that don't overridegetEnvironmentId()simply report no environment ID.TrackSeriesContextis unaffected.Link to Devin session: https://app.devin.ai/sessions/bfe54128e2804a96bb100e6120e9a3ef
Requested by: @kinyoklion
Note
Overview
Adds LaunchDarkly environment ID to evaluation hooks by threading the
x-ld-envidvalue through flag payloads and stores, then exposing it onEvaluationSeriesContext.environmentId.Data path:
FullDataSet(and FDv2 change sets) now carryenvironmentId; FDv1 polling and streaming attach it from successful response headers. Stores implementDataStore.getEnvironmentId()(in-memory, write-through, persistent wrapper) with retention rules—empty/absent values do not clear a previously set ID.LDClientwiresEvaluatorWithHookswithdataSystem::getEnvironmentId; lookup failures are logged and evaluations still run withnull.Interop: FDv1→FDv2 adapter retains environment ID across partial updates; contract tests declare
hook-environment-idand forwardenvironmentIdin hook callbacks.Backward compatible: new
FullDataSetconstructor parameter with existing constructors delegating; defaultgetEnvironmentId()isnullfor custom stores.Reviewed by Cursor Bugbot for commit dc87015. Bugbot is set up for automated code reviews on this repo. Configure here.