fix(openfeature): Rust provider improvements#1094
Conversation
|
Review changes with Changed Files
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughRefresh configuration now uses millisecond-based constructors and accessors. Provider refreshes support timeouts and stale-status recovery, value conversion handles null object fields, typed array resolution is added, and provider error variants are refined. ChangesProvider refresh semantics
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR refines the Rust OpenFeature provider implementation to better align behavior and ergonomics with the OpenFeature spec and the other language providers, especially around refresh strategies, typed resolution, and error reporting.
Changes:
- Reworked refresh strategy durations to be millisecond-based (with deprecated seconds fields preserved) and added refresh timeout handling + staleness tracking on refresh failure.
- Improved typed resolution by adding
resolve_arrayand tightening JSON-to-OpenFeature conversions (object-onlyStructValue, null handling). - Simplified/streamlined provider option types and error taxonomy (e.g.,
DataSourceError,RefreshError), and updated examples accordingly.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/superposition_provider/src/types.rs | Introduces ms-based refresh strategy APIs with deprecated seconds fields; expands error enum; removes evaluation-cache-related option structs; adds unit tests. |
| crates/superposition_provider/src/traits.rs | Adds typed resolve_array and documents typed-resolution semantics vs other SDKs. |
| crates/superposition_provider/src/remote_provider.rs | Changes experiment-variant lookup request identifier handling. |
| crates/superposition_provider/src/provider.rs | Updates configuration construction to match removed evaluation-cache parameter. |
| crates/superposition_provider/src/local_provider.rs | Uses ms durations, adds refresh timeout bounding, and records provider staleness on refresh failure. |
| crates/superposition_provider/src/lib.rs | Updates tests for the new ConfigurationOptions::new signature. |
| crates/superposition_provider/src/data_source/file.rs | Reclassifies file-source failures as DataSourceError. |
| crates/superposition_provider/src/conversions.rs | Makes StructValue conversion object-only; drops null object fields; adds tests. |
| crates/superposition_provider/src/client.rs | Updates polling/on-demand logic to ms-based durations and renames TTL params accordingly. |
| crates/superposition_provider/examples/polling_example.rs | Updates example to use ms-based polling env var and PollingStrategy::new. |
| crates/superposition_provider/examples/local_with_fallback_example.rs | Updates example to use PollingStrategy::new (ms). |
| crates/superposition_provider/examples/local_http_example.rs | Updates example to use PollingStrategy::new (ms). |
| crates/superposition_provider/examples/local_file_example.rs | Updates example to use OnDemandStrategy::new (ms). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .workspace_id(&self.options.workspace_id) | ||
| .org_id(&self.options.org_id) | ||
| .set_context(Some(query_data)) | ||
| .identifier(targeting_key) | ||
| .identifier(targeting_key.unwrap_or_default()) | ||
| .set_prefix(prefix_filter) |
| let is_elapsed = |cached_at: DateTime<Utc>| { | ||
| (chrono::Utc::now() - cached_at).num_seconds() > ttl as i64 | ||
| (chrono::Utc::now() - cached_at).num_milliseconds() > ttl as i64 | ||
| }; |
| let should_refresh = match *last_updated { | ||
| Some(last) => (now - last).num_seconds() > ttl as i64, | ||
| Some(last) => (now - last).num_milliseconds() > ttl_ms as i64, | ||
| None => true, | ||
| }; |
| let should_refresh = match *last_updated { | ||
| Some(last) => (now - last).num_seconds() > ttl as i64, | ||
| Some(last) => (now - last).num_milliseconds() > ttl_ms as i64, | ||
| None => true, | ||
| }; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/superposition_provider/src/client.rs (2)
385-484: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSame missing-timeout gap as
CacConfig(lines 109-184).
ExperimentationConfig::start_polling/on_demand_configcallget_experiments_static/get_experiment_groups_staticdirectly with no timeout bound at all (not even logged here). Same fix applies: threadtimeout_ms()through and wrap the fetch withtokio::time::timeout.🤖 Prompt for 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. In `@crates/superposition_provider/src/client.rs` around lines 385 - 484, Update ExperimentationConfig::start_polling and on_demand_config to obtain the configured timeout via timeout_ms(), pass it through the get_experiments_static/get_experiment_groups_static fetch path, and wrap each fetch with tokio::time::timeout. Handle timeout errors consistently with existing fetch errors while preserving the current cache update and stale-data behavior.
109-184: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEnforce the refresh timeout around each fetch. The timeout values are only logged here;
start_pollingandon_demand_configstill await the fetch directly, so a hung request can stall polling or block an on-demand caller indefinitely. Apply the sametokio::time::timeoutaround the config fetch in both CAC and experimentation refresh paths.🤖 Prompt for 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. In `@crates/superposition_provider/src/client.rs` around lines 109 - 184, Wrap each config fetch in start_polling and on_demand_config with tokio::time::timeout using the configured refresh timeout, handling timeout errors through the existing polling and stale-config/error paths. Apply the same timeout behavior to the corresponding CAC and experimentation refresh implementations, preserving successful cache updates and on-demand return behavior.
🧹 Nitpick comments (1)
crates/superposition_provider/src/types.rs (1)
69-79: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo lower-bound validation on millisecond durations in
PollingStrategy::new/OnDemandStrategy::new.Both constructors accept an arbitrary
u64forinterval_milliseconds/ttl_millisecondswith no minimum-value guard. A0(reachable e.g. viaPOLL_INTERVAL_MS=0inexamples/polling_example.rs, which parses straight intou64) flows intostart_polling'ssleep(Duration::from_millis(interval_ms))(local_provider.rs) producing an effectively unthrottled polling loop against the origin endpoint; similarly a0TTL makesensure_fresh_datatreat cache as expired on almost every call. A small validation/clamp in the constructors would close this gap cheaply.♻️ Example guard
pub fn new(interval_milliseconds: u64, timeout_milliseconds: Option<u64>) -> Self { + debug_assert!(interval_milliseconds > 0, "polling interval must be > 0ms"); #[allow(deprecated)] Self {Also applies to: 127-142
🤖 Prompt for 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. In `@crates/superposition_provider/src/types.rs` around lines 69 - 79, Update PollingStrategy::new and OnDemandStrategy::new to validate or clamp interval_milliseconds and ttl_milliseconds so zero values cannot be stored. Ensure both constructors enforce a positive minimum before these durations reach start_polling or ensure_fresh_data, while preserving existing handling of valid nonzero values.
🤖 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.
Outside diff comments:
In `@crates/superposition_provider/src/client.rs`:
- Around line 385-484: Update ExperimentationConfig::start_polling and
on_demand_config to obtain the configured timeout via timeout_ms(), pass it
through the get_experiments_static/get_experiment_groups_static fetch path, and
wrap each fetch with tokio::time::timeout. Handle timeout errors consistently
with existing fetch errors while preserving the current cache update and
stale-data behavior.
- Around line 109-184: Wrap each config fetch in start_polling and
on_demand_config with tokio::time::timeout using the configured refresh timeout,
handling timeout errors through the existing polling and stale-config/error
paths. Apply the same timeout behavior to the corresponding CAC and
experimentation refresh implementations, preserving successful cache updates and
on-demand return behavior.
---
Nitpick comments:
In `@crates/superposition_provider/src/types.rs`:
- Around line 69-79: Update PollingStrategy::new and OnDemandStrategy::new to
validate or clamp interval_milliseconds and ttl_milliseconds so zero values
cannot be stored. Ensure both constructors enforce a positive minimum before
these durations reach start_polling or ensure_fresh_data, while preserving
existing handling of valid nonzero values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef121c14-1263-4493-a10a-877406f26180
📒 Files selected for processing (13)
crates/superposition_provider/examples/local_file_example.rscrates/superposition_provider/examples/local_http_example.rscrates/superposition_provider/examples/local_with_fallback_example.rscrates/superposition_provider/examples/polling_example.rscrates/superposition_provider/src/client.rscrates/superposition_provider/src/conversions.rscrates/superposition_provider/src/data_source/file.rscrates/superposition_provider/src/lib.rscrates/superposition_provider/src/local_provider.rscrates/superposition_provider/src/provider.rscrates/superposition_provider/src/remote_provider.rscrates/superposition_provider/src/traits.rscrates/superposition_provider/src/types.rs
💤 Files with no reviewable changes (2)
- crates/superposition_provider/src/lib.rs
- crates/superposition_provider/src/provider.rs
b9c1249 to
e93f729
Compare
e93f729 to
96fed3d
Compare
Change log
Improvements and fixes in rust provider to make it more consistent with openfeature spec and other languages in parallel
Summary by CodeRabbit
New Features
Bug Fixes
Documentation