feat(flagd): support custom gRPC metadata on in-process SyncFlags - #404
feat(flagd): support custom gRPC metadata on in-process SyncFlags#404tuhinkanti wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughFlagd now accepts optional synchronous gRPC client interceptors and custom channel credentials. Config normalizes interceptors, the provider forwards them, and both gRPC resolvers apply them. Unexpected stream errors now trigger logging and reconnect backoff. ChangesFlagd gRPC Interceptors
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change adds configurable synchronous gRPC interceptors and channel credentials, applies them to both flagd resolver paths, and adds stream reconnect handling. No concrete merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant FlagdProvider
participant Config
participant GrpcResolver
participant GrpcWatcher
participant FlagSyncServiceStub
FlagdProvider->>Config: pass client_interceptors
Config->>GrpcResolver: provide normalized interceptors
Config->>GrpcWatcher: provide normalized interceptors
GrpcResolver->>GrpcResolver: create and wrap channel
GrpcWatcher->>GrpcWatcher: create and wrap channel
GrpcWatcher->>FlagSyncServiceStub: call SyncFlags through interceptor
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
Full details: Linked Issues checkExplanation The changes do not implement the primary API requirements from issue Resolution Implement sync_metadata in Config and FlagdProvider as a sequence of header pairs. Update _create_metadata in the in-process resolver to preserve flagd-selector and append configured metadata. Keep the existing empty-metadata behavior when neither option is configured. Add tests for selector-only, custom-only, combined, and default cases. Full details: Out of Scope Changes checkExplanation The pull request includes changes unrelated to issue Full details: Docstring CoverageExplanation Docstring coverage is 5.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 35 functions across 7 files. (1 skipped: 1 unsupported.)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@providers/openfeature-provider-flagd/src/openfeature/contrib/provider/flagd/config.py`:
- Line 107: Preserve positional constructor compatibility by moving
sync_metadata after fatal_status_codes in both public constructors:
providers/openfeature-provider-flagd/src/openfeature/contrib/provider/flagd/config.py
lines 107-107 and
providers/openfeature-provider-flagd/src/openfeature/contrib/provider/flagd/provider.py
lines 68-68. Add a regression test confirming positional fatal_status_codes
arguments still configure fatal status handling correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 67aff781-dac2-4bef-b0b2-89d407bc7082
📒 Files selected for processing (5)
providers/openfeature-provider-flagd/src/openfeature/contrib/provider/flagd/config.pyproviders/openfeature-provider-flagd/src/openfeature/contrib/provider/flagd/provider.pyproviders/openfeature-provider-flagd/src/openfeature/contrib/provider/flagd/resolvers/process/connector/grpc_watcher.pyproviders/openfeature-provider-flagd/tests/test_config.pyproviders/openfeature-provider-flagd/tests/test_grpc_watcher.py
Add a `sync_metadata` option to FlagdProvider / Config that appends user-supplied gRPC metadata headers to every in-process SyncFlags call, alongside the provider-managed flagd-selector header. This lets callers inject infrastructure-specific headers on the long-lived sync stream -- e.g. `x-envoy-upstream-rq-timeout-ms: 0` to disable a proxy request timeout that would otherwise sever the stream after its default deadline. Keep `fatal_status_codes` as the last positional parameter in both public constructors so existing positional callers are unaffected, and add a regression test covering that ordering. Signed-off-by: Tuhin Sharma <tsharma@salesforce.com>
fd2c118 to
9dcd715
Compare
|
Thanks for this; the problem is valid, proxies killing the long-lived sync stream is a realistic issue. My worry is that Since grpcio already handles this, what about exposing a gRPC interceptor option instead? An interceptor that adds your Docs:
Would you be up for reworking it that way? Happy to help. |
|
Let me look and get back to you |
Keep both sync_metadata test setup and the new fatal_status_codes / reconnect backoff coverage from main.
Signed-off-by: Tuhin Sharma <tsharma@salesforce.com>
268ad37 to
81adc6e
Compare
Signed-off-by: Tuhin Sharma <tsharma@salesforce.com>
4 participants
This PR
Closes #403
Adds a
sync_metadataoption to the flagd provider so callers can send custom gRPC metadata headers on the in-processSyncFlagsstream.Problem
The in-process resolver builds the gRPC metadata for the long-lived
SyncFlagscall in_create_metadata, which adds only theflagd-selectorheader (and only when a selector is set). There is no way for a caller to add other metadata headers, so infrastructure-specific headers cannot be sent on the sync stream.Change
sync_metadataoption toFlagdProviderandConfig(a sequence of(key, value)header pairs)._create_metadata, append the configured pairs to the metadata, alongside theflagd-selectorheader.Use case
A proxy can close a stream after a default request timeout. Sending a header such as
x-envoy-upstream-rq-timeout-ms: 0on the long-lived sync stream keeps the stream open.Compatibility
Backward compatible. Custom metadata is added only when
sync_metadatais set. The default behavior does not change.Tests
New tests cover config passthrough and metadata construction (selector-only, custom-only, both together, and disabled).
Full suite for the two touched test files: