feat: Add environment ID support for hooks. - #81
Merged
Conversation
kinyoklion
commented
Mar 4, 2025
| LastError = null | ||
| }; | ||
| _status = new StateMonitor<DataSourceStatus, StateAndError>(initialStatus, MaybeUpdateStatus, _log); | ||
| _status = new StateMonitor<DataSourceStatus, StateAndError>(initialStatus, MaybeUpdateStatus, _log); |
Member
Author
There was a problem hiding this comment.
Just a whitespace change.
kinyoklion
commented
Mar 4, 2025
| #region IDataSourceUpdatesHeaders methods | ||
| public bool InitWithHeaders(FullDataSet<ItemDescriptor> allData, IEnumerable<KeyValuePair<string, IEnumerable<string>>> headers) | ||
| { | ||
| ImmutableDictionary<DataKind, ImmutableDictionary<string, ItemDescriptor>> oldData = null; |
Member
Author
There was a problem hiding this comment.
Most of the method body is from the original init.
kinyoklion
marked this pull request as ready for review
March 4, 2025 18:16
tanderson-ld
reviewed
Mar 5, 2025
tanderson-ld
left a comment
Contributor
There was a problem hiding this comment.
Approved. Putting this in a comment to avoid accidental merge as the description says this is pending other work.
tanderson-ld
approved these changes
Mar 19, 2025
kinyoklion
pushed a commit
that referenced
this pull request
Mar 21, 2025
🤖 I have created a release *beep* *boop* --- ## [8.7.0](LaunchDarkly.ServerSdk-v8.6.0...LaunchDarkly.ServerSdk-v8.7.0) (2025-03-20) ### Features * Add environment ID support for hooks. ([#81](#81)) ([49149f2](49149f2)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
3 tasks
kinyoklion
added a commit
to launchdarkly/python-server-sdk
that referenced
this pull request
Aug 13, 2026
**Requirements** - [x] I have added test coverage for new or changed functionality - [x] I have followed the repository's [pull request submission guidelines](../blob/main/CONTRIBUTING.md#submitting-pull-requests) - [x] I have validated my changes against all supported platform versions **Related issues** Implements the `environmentId` field of `EvaluationSeriesContext` from the [hooks spec](https://github.com/launchdarkly/sdk-specs/blob/main/specs/HOOK-hooks/README.md), which the OTel tracing hook uses for `feature_flag.set.id`. Mirrors launchdarkly/dotnet-core#81. **Describe the solution you've provided** `EvaluationSeriesContext` gains an optional `environment_id`, populated from the `X-LD-EnvID` response header sent by LaunchDarkly. Both data systems are supported, and each exposes it through a new `DataSystem.environment_id` property that the client reads when building the series context. Per the spec, it is only recorded from successful responses, so hooks never see an environment ID scraped off an error page. - FDv2 already parsed the header into `Basis.environment_id` / `Update.environment_id` but discarded it; `FDv2` now latches the last value seen on a success path (a `_Success` basis, or an update whose state is `VALID`). - FDv1 had no access to the header. Streaming reads it from the `Start` action of the SSE client, polling reads it from the response headers, and both record it on `DataSourceUpdateSinkImpl`. Sketch of the FDv1 path: ```python # streaming if isinstance(action, Start): record_environment_id(self._data_source_update_sink, action.headers) # polling (all_data, headers) = self._get_all_data_with_headers() record_environment_id(self._data_source_update_sink, headers) ``` **Describe alternatives you've considered** Following the .NET implementation more literally, where the environment ID is stored as init metadata on the data store, would require optional extension interfaces on the public `FeatureStore`/`DataSourceUpdateSink` types. Instead, header handling stays inside the data sources and is surfaced by the data system, so externally implemented stores, sinks, feature requesters, and update processors continue to work unchanged (they simply report no environment ID). **Additional context** - No change is needed in `launchdarkly-eventsource`: it already exposes response headers on `Start` and `Fault` (unlike the .NET event source, which needed launchdarkly/dotnet-eventsource#104). - The async (experimental) client does not run hooks yet, so the async data source path is untouched. - The `environmentId` support in the OTel tracing hook lives in `python-server-sdk-otel` and is a follow-up. Link to Devin session: https://app.devin.ai/sessions/bfe54128e2804a96bb100e6120e9a3ef Requested by: @kinyoklion <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Adds optional **`environment_id`** on **`EvaluationSeriesContext`** so evaluation hooks (e.g. OTel **`feature_flag.set.id`**) can see which LaunchDarkly environment the SDK is connected to, matching the hooks spec. > > The ID comes from the **`X-LD-EnvID`** header on **successful** data-source responses only—not from error responses. **FDv1** records it via streaming **`Start`** headers and polling response headers into **`DataSourceUpdateSinkImpl`**; **FDv2** latches values already present on successful **`Basis`** / **`VALID`** **`Update`** objects. **`DataSystem.environment_id`** exposes the value and **`LDClient`** passes it when building hook series context. > > Custom **`FeatureRequester`** implementations without **`get_all_data_with_headers`** still work (no env ID). Contract tests add the **`hook-environment-id`** capability. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 7ce7b6a. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Devin AI <devin-ai-integration[bot]@users.noreply.github.com>
3 tasks
kinyoklion
added a commit
to launchdarkly/ruby-server-sdk
that referenced
this pull request
Aug 20, 2026
**Requirements** - [x] I have added test coverage for new or changed functionality - [x] I have followed the repository's [pull request submission guidelines](../blob/main/CONTRIBUTING.md#submitting-pull-requests) - [x] I have validated my changes against all supported platform versions **Related issues** Part of the cross-SDK work to expose the LaunchDarkly environment ID on hook contexts, per the [hooks](https://github.com/launchdarkly/sdk-specs/blob/main/specs/HOOK-hooks/README.md) and [OTEL](https://github.com/launchdarkly/sdk-specs/blob/main/specs/OTEL-openteletry-integration/README.md) specs. Equivalent implementations: launchdarkly/dotnet-core#81, launchdarkly/python-server-sdk#484, launchdarkly/cpp-sdks#594. **Describe the solution you've provided** `EvaluationSeriesContext` gains an optional `environment_id`, populated by `LDClient` from the active data system: ```ruby Interfaces::Hooks::EvaluationSeriesContext.new(key, context, default, method, @data_system.environment_id) ``` Both data systems now report it, so the value is only visible once LaunchDarkly has actually answered: - FDv1 — `X-LD-EnvID` is read from the streaming connection response headers (`SSE::Client#on_connect`) and from polling responses (`Requestor#request_all_data_with_headers`), then recorded on the data source `UpdateSink`, which the FDv1 data system exposes. - FDv2 — the ID was already parsed into `Basis`/`Update` by the polling and streaming data sources but discarded; the FDv2 data system now latches it from a successful initializer basis and from `VALID` synchronizer updates. This also covers the FDv1 fallback synchronizer, which reports headers the same way. Error responses, missing headers, and empty header values are ignored and never clear a previously known ID. The contract test service reports `environmentId` on the evaluation series context and declares the `hook-environment-id` capability. **Describe alternatives you've considered** A shared environment-ID holder threaded into each data source was rejected as a side channel; carrying the value with the data/response metadata matches the other SDKs and the existing FDv2 `Basis`/`Update` shape. **Additional context** - No `ld-eventsource` change is needed: `SSE::Client#on_connect` already yields the successful response headers on every connection and reconnection. - `hooks/evaluation/provides the environment ID` passes against the released v2.39.0 harness (default/streaming and polling modes). The repo's v3 contract-test run is pinned to `v3.0.0-alpha.6`, which predates that test; bumping the pin can be a separate `ci:` change. - `ruby-server-sdk-otel`'s tracing hook only uses the configured environment ID today; adding the series-context fallback is a follow-up in that repo. Link to Devin session: https://app.devin.ai/sessions/bfe54128e2804a96bb100e6120e9a3ef Requested by: @kinyoklion <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Evaluation hooks now get an optional `environment_id` on `EvaluationSeriesContext`, filled from the active data system after LaunchDarkly reports it. > > **FDv1** reads `X-LD-EnvID` from streaming connect headers and polling responses (`request_all_data_with_headers`) and stores it on the data-source update sink. **FDv2** latches the ID from a successful initializer `Basis` and from `VALID` synchronizer updates (it was already on those types but unused). Missing, empty, or error-path values are ignored and never clear a known ID. > > Contract tests declare `hook-environment-id` and include `environmentId` in hook callbacks. CI v3 contract tests now pin to `v3` instead of `v3.0.0-alpha.6`. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit a713037. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
3 tasks
kinyoklion
pushed a commit
that referenced
this pull request
Sep 3, 2026
**Requirements** - [x] I have added test coverage for new or changed functionality - [x] I have followed the repository's [pull request submission guidelines](../blob/main/CONTRIBUTING.md#submitting-pull-requests) - [x] I have validated my changes against all supported platform versions **Related issues** Fixes #344. Thanks to @cda210 for the report and for pinpointing the root cause — this PR is the one-line fix they proposed, plus a regression test. **Describe the solution you've provided** `FeatureRequestor.GetAllDataAsync()` signals "payload unchanged" by returning a `null` `DataSetWithHeaders` (its doc comment: *"or null if they have not been modified"*). `PollingDataSource.UpdateTaskAsync()` dereferenced `dataAndHeaders.DataSet` without first checking the wrapper reference, so every poll that received an HTTP 304 threw a `NullReferenceException`. It was swallowed by the generic `catch (Exception)`, logged at `Warning`, and reported as `DataSourceState.Interrupted`. The guard now checks the wrapper before the payload: ```csharp if (dataAndHeaders is null || dataAndHeaders.DataSet is null) ``` Worth noting the impact is slightly worse than "spurious warnings": because *every* unchanged poll takes this path, a polling client whose flags are stable sits **permanently** in `Interrupted` after its second poll, which matters for anyone gating a health check on `DataSourceStatusProvider`. Flag evaluation is unaffected, since the store already holds data from the prior successful poll. **Origin of the regression** Introduced in #81 (environment ID support for hooks), which wrapped the requestor's return value in `DataSetWithHeaders` so headers could ride along. That turned one nullable into two — the outer reference and the inner `FullDataSet<ItemDescriptor>?` — and the caller's check was rewritten onto the inner one while the requestor kept signalling 304 via the outer one: https://github.com/launchdarkly/dotnet-core/pull/81/changes#diff-eff45aea6b7a68df4fc34a2d803960905666dd3995eea912053b1a7171129821R65 (Stable permalink to the same line, in case the diff anchor drifts: [`PollingDataSource.cs#L65` @ `49149f2`](https://github.com/launchdarkly/dotnet-core/blob/49149f2509a4a94e77f36414708d73e368b0acc5/pkgs/sdk/server/src/Internal/DataSources/PollingDataSource.cs#L65).) Before that change the check was simply `if (allData is null)`, against a `Task<FullDataSet<ItemDescriptor>?>` return type. The bug has therefore been present since **8.7.0**. **Describe alternatives you've considered** - **`if (dataAndHeaders is null)` alone** — this is the strictly minimal inversion of #81, since the inner `DataSet is null` arm is provably unreachable: `new DataSetWithHeaders(...)` is constructed in exactly one place and always with a non-null `FullDataSet` value, `FeatureRequestor` is the only implementation of `IFeatureRequestor`, and an empty response body throws `JsonException` rather than yielding a null `DataSet`. I kept the disjunction to match the fix proposed in the issue and because it stays correct if a second implementation is ever added. Happy to tighten it if reviewers prefer. - **Fixing it in the requestor instead**, by returning `new DataSetWithHeaders(null, null)` on a 304 so the existing check becomes correct. Rejected: a wrapper carrying a null payload is a weaker contract than a null wrapper, and it would contradict the requestor's documented return value. **Additional context** The existing 304 coverage (`InitIsNotRepeatedIfServerReturnsNotModifiedStatus`, `ResponseWithNewEtagUpdatesEtag`) could not catch this — those tests assert only that `Init` is not re-called and that ETags are echoed, and a thrown `NullReferenceException` produces "no `Init`" just as successful 304 handling does. The new test asserts the parts that were actually wrong: that every published status is `Valid` with no `LastError`, and that nothing is logged at `Warning`. Verified: the new test fails on `main` with `Interrupted` / `NullReferenceException` and passes with the fix. Full server SDK unit suite green — 1606/1606 on net8.0. The FDv2 polling data source is not affected (`FDv2PollingDataSource` handles `response == null` explicitly), nor is the client-side SDK (`FeatureFlagRequestor` returns a non-null `WebResponse(304, null, ...)`). <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Fixes a **NullReferenceException** when the polling data source receives unchanged flag data (HTTP **304**): `GetAllDataAsync()` returns a **null** `DataSetWithHeaders`, but `UpdateTaskAsync` only checked `DataSet` and dereferenced the wrapper first. > > The guard is now `dataAndHeaders is null || dataAndHeaders.DataSet is null`, so unchanged polls report **`DataSourceState.Valid`** instead of being caught as a generic failure and left in **`Interrupted`** with warning logs. > > Adds **`StatusRemainsValidIfServerReturnsNotModifiedStatus`** to lock in valid status, no `LastError`, and no “polling failed” warnings across repeated 304s. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 6c932e1. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR pipes the support for the environment ID header from the event source, through the data sources, and into the data store, and then exposes it to the evaluation series hook context.
Each step can optionally support the environment ID/headers for compatibility.
This PR will be held for back-end support and testing.