Skip to content

feat: Add environment ID support for hooks - #202

Open
kinyoklion wants to merge 3 commits into
mainfrom
devin/1786661026-java-hook-environment-id
Open

feat: Add environment ID support for hooks#202
kinyoklion wants to merge 3 commits into
mainfrom
devin/1786661026-java-hook-environment-id

Conversation

@kinyoklion

@kinyoklionkinyoklion commented Aug 13, 2026

Copy link
Copy Markdown
Member

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

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

EvaluationSeriesContext gains an environmentId field, populated from the X-LD-EnvID response header LaunchDarkly sends on flag delivery responses.

  • The environment ID travels with the flag data instead of through a separate sink call — FullDataSet carries it for FDv1, and FDv2 change sets already did.
  • Stores retain it: DataStore.getEnvironmentId() (default null) is implemented by InMemoryDataStore, WriteThroughStore and PersistentDataStoreWrapper; the data systems read it from the store for the hook context.
  • Capture points: FDv1 polling (DefaultFeatureRequestor, after the response is confirmed successful), FDv1 streaming (StreamProcessor, from the put event's stream headers), FDv2 initializers/synchronizers, and the FDv1 fallback adapter used under FDv2.
  • Empty or absent values never clear a retained ID, and nothing is exposed before data has been applied.
  • The contract test service reports environmentId and declares hook-environment-id.

Backend-only change, so no screenshots or staging preview apply.

Describe alternatives you've considered

A DataSourceUpdateSink.setEnvironmentId call (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

EvaluatorWithHooks receives a supplier so each evaluation sees the current value:

newEvaluationSeriesContext(method, featureKey, context, defaultValue, environmentIdSupplier.get())

FullDataSet gains a third constructor parameter (environmentId); the existing constructors delegate to it, so this is source and binary compatible for existing callers. Conversions that rebuild a FullDataSet (dependency sorting, persistent store serialization, change-set-to-legacy-init) now preserve the value.

How to test

  • ./gradlew test checkstyleMain checkstyleTest javadoc in lib/sdk/server
  • hooks/evaluation/provides the environment ID passes against released harness v2.39.0 (FDv1, default and polling) and v3.2.0-alpha.6 (FDv2); full suites also pass.

Risks / follow-ups

  • The repo's v3 contract test run is pinned to v3.0.0-alpha.6, so CI won't exercise the new test until that pin is bumped.
  • Custom DataStore implementations that don't override getEnvironmentId() simply report no environment ID.
  • Track hooks are not implemented in this SDK, so TrackSeriesContext is unaffected.

Link to Devin session: https://app.devin.ai/sessions/bfe54128e2804a96bb100e6120e9a3ef
Requested by: @kinyoklion


Note

Overview
Adds environmentId to EvaluationSeriesContext so evaluation hooks can see which LaunchDarkly environment the flag data came from, sourced from the X-LD-EnvID header on flag delivery responses.

The ID is carried with flag payloads rather than set through a separate sink: FullDataSet and change sets preserve it through sorting, persistence conversion, and FDv1→FDv2 adaptation. Stores retain it via DataStore.getEnvironmentId() (in-memory, write-through, and persistent wrapper); empty or missing values do not clear a previously stored ID. Capture happens on successful FDv1 poll and stream put events, plus existing FDv2 paths; LDClient passes dataSystem::getEnvironmentId into EvaluatorWithHooks on each evaluation. Contract tests add hook-environment-id and report environmentId in hook callbacks.

Reviewed by Cursor Bugbot for commit fc78a70. Bugbot is set up for automated code reviews on this repo. Configure here.

Co-Authored-By: rlamb@launchdarkly.com <4955475+kinyoklion@users.noreply.github.com>
@kinyoklionkinyoklion self-assigned this Aug 13, 2026
@devin-ai-integration

Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@devin-ai-integrationdevin-ai-integrationBot added the devin-pr Label for PR's created using devin. label Aug 13, 2026
@devin-ai-integration

Copy link
Copy Markdown
Contributor

@cursor review

*
* @param environmentId the environment ID reported by LaunchDarkly
*/
default void setEnvironmentId(String environmentId) {}

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds good — leaving it as is.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I prefer approaches where it goes through to the store with the flag data and perfer one operation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@kinyoklion
kinyoklion marked this pull request as ready for review August 14, 2026 21:35
@kinyoklion
kinyoklion requested a review from a team as a code ownerAugust 14, 2026 21:35
Co-Authored-By: rlamb@launchdarkly.com <4955475+kinyoklion@users.noreply.github.com>

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ 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 2ab6efb. Configure here.

…re init
Co-Authored-By: rlamb@launchdarkly.com <4955475+kinyoklion@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

devin-prLabel for PR's created using devin.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@kinyoklion@tanderson-ld