feat(config): add feature flags for runtime endpoint gating#127
Open
SudipSinha wants to merge 4 commits into
Open
feat(config): add feature flags for runtime endpoint gating#127SudipSinha wants to merge 4 commits into
SudipSinha wants to merge 4 commits into
Conversation
Contributor
Reviewer's GuideIntroduce centralized feature-flag-based endpoint gating for fairness, drift, and explainer routers, with deployment-time env overrides and helper functions that keep disabled routers out of the FastAPI app and OpenAPI docs, plus tests and a small lm_evaluation_harness import handling tweak. Sequence diagram for endpoint registration with feature flagssequenceDiagram
actor Operator
participant Env as EnvVars
participant FeatureFlags as FeatureFlagsModule
participant Registry as RegistryModule
participant App as FastAPIApp
participant FairnessRouter as Fairness_dir_router
Operator->>Env: Configure TRUSTYAI_ENABLE_FAIRNESS, TRUSTYAI_ENABLE_FAIRNESS_DIR
Env->>FeatureFlags: Access TRUSTYAI_ENABLE_FAIRNESS
FeatureFlags->>FeatureFlags: _flag(name=fairness, default=true)
FeatureFlags-->>Env: bool for fairness
Env->>FeatureFlags: Access TRUSTYAI_ENABLE_FAIRNESS_DIR
FeatureFlags->>FeatureFlags: _flag(name=fairness_dir, default=true)
FeatureFlags-->>Env: bool for fairness_dir
FeatureFlags->>FeatureFlags: Build ENDPOINTS dict
App->>Registry: register_if_enabled_with_group(app, FairnessRouter, fairness, fairness_dir, tag)
Registry->>FeatureFlags: _is_enabled(flag=fairness)
FeatureFlags-->>Registry: group_enabled
alt fairness group disabled
Registry-->>App: Skip registration (log debug)
else fairness group enabled
Registry->>FeatureFlags: _is_enabled(flag=fairness_dir)
FeatureFlags-->>Registry: metric_enabled
alt fairness_dir metric disabled
Registry-->>App: Skip registration (log debug)
else fairness_dir metric enabled
Registry->>App: _include_router(app, FairnessRouter, tag, prefix)
App-->>Registry: Router included in app and OpenAPI
end
end
Class diagram for feature flag configuration and router registration helpersclassDiagram
class FeatureFlagsModule {
<<module>>
-logging logger
-frozenset _TRUTHY
-frozenset _FALSY
+bool _flag(name, default)
+dict~str, bool~ ENDPOINTS
}
class RegistryModule {
<<module>>
-logging logger
+bool _is_enabled(flag)
+_include_router(app, router, tag, prefix)
+register_if_enabled(app, router, flag, tag, prefix)
+register_if_enabled_with_group(app, router, group_flag, metric_flag, tag, prefix)
}
class FastAPIApp {
+include_router(router, tags, prefix)
}
class APIRouter {
<<FastAPI.APIRouter>>
}
class MainModule {
<<module>>
+lifespan(_app)
+fairness routers
+drift routers
+explainer routers
}
FeatureFlagsModule "1" --> "1" RegistryModule : provides ENDPOINTS used_by
RegistryModule --> FastAPIApp : calls include_router
RegistryModule --> APIRouter : conditionally registers routers
MainModule --> RegistryModule : uses register_if_enabled
MainModule --> RegistryModule : uses register_if_enabled_with_group
MainModule --> FeatureFlagsModule : imports ENDPOINTS via RegistryModule
MainModule --> FastAPIApp : configures app
MainModule --> APIRouter : defines metric routers
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
SudipSinha
changed the title
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR implements a feature-flag system for conditionally gating API endpoints in a FastAPI service. It introduces environment-driven configuration via the ChangesEndpoint Gating via Feature Flags
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
…-55917] Migrate from always-on endpoints to a feature flag system that mirrors Java's @EndpointDisabled pattern. Each metric group (fairness, drift, explainer) has a master gate plus individual metric flags that can be overridden at deployment time via TRUSTYAI_ENABLE_<NAME> env vars. New helpers: register_if_enabled() for single-gate endpoints and register_if_enabled_with_group() for group-gated metrics. Explainer endpoints now use the group gate like drift and fairness do. Also fixes pyrefly unbound-name error for lm_evaluation_harness_router. Co-Authored-By: Claude Opus 4.7 <[email protected]>
- Fix ruff lint violations (FBT001/FBT003, D104, ERA001, D417, ANN201, D101/D102) - Make _flag() default parameter keyword-only - Add missing __init__.py with docstrings for new packages - Add return type annotations and docstrings to all test methods - Add PLR0913 per-file-ignore for registry.py (6-param group-gated API) - Replace dict[str, Any] with dict[str, str | list[str]] in registry.py - Use ruff-formatted imports consistent with main branch Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Remove unused `data_download` flag (dead code, no router wired) - Extract `_is_enabled` and `_include_router` helpers in registry.py to eliminate duplication between `register_if_enabled` and `_with_group` - Add integration tests verifying disabled flags remove endpoints from OpenAPI: group-level (fairness, drift) and individual metric gating Co-Authored-By: Claude Opus 4.6 <[email protected]>
SudipSinha
force-pushed
the
feature-flags-metrics
branch
from
f46f325 to
987cb18
Compare
4 tasks
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
registry._is_enabled, silently treating unknown flag names asFalsecould hide configuration or typo issues; consider logging a warning (or even raising in debug modes) when a flag key is not present inENDPOINTSso misconfigurations are surfaced early. - The
ENDPOINTSdict and its keys are now a central contract used acrossmain.py,registry, and tests; you might want to centralize these flag names (e.g., constants or an Enum) to avoid stringly-typed usage and reduce the risk of typos when adding new endpoints.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `registry._is_enabled`, silently treating unknown flag names as `False` could hide configuration or typo issues; consider logging a warning (or even raising in debug modes) when a flag key is not present in `ENDPOINTS` so misconfigurations are surfaced early.
- The `ENDPOINTS` dict and its keys are now a central contract used across `main.py`, `registry`, and tests; you might want to centralize these flag names (e.g., constants or an Enum) to avoid stringly-typed usage and reduce the risk of typos when adding new endpoints.
## Individual Comments
### Comment 1
<location path="src/service/config/feature_flags.py" line_range="35-44" />
<code_context>
+ return default
+
+
+ENDPOINTS: dict[str, bool] = {
+ "fairness": _flag("fairness", default=True),
+ "fairness_spd": _flag("fairness_spd", default=True),
+ "fairness_dir": _flag("fairness_dir", default=True),
+ "drift": _flag("drift", default=True),
+ "drift_ks_test": _flag("drift_ks_test", default=True),
+ "drift_jensen_shannon": _flag("drift_jensen_shannon", default=True),
+ "drift_compare_means": _flag("drift_compare_means", default=True),
+ "explainer": _flag("explainer", default=False),
+ "explainer_local": _flag("explainer_local", default=False),
+ "explainer_global": _flag("explainer_global", default=False),
+}
</code_context>
<issue_to_address>
**question (bug_risk):** Clarify or reconsider the default-disabled behavior for explainer endpoints relative to other groups.
Fairness and drift endpoints default to enabled, while explainer endpoints default to disabled. If that asymmetry is intentional (e.g., explainer endpoints are experimental/expensive), consider documenting it near these flags or in higher-level config to avoid operator confusion. If it’s not intentional, aligning explainer defaults to `True` would keep existing behavior and prevent explainer endpoints from disappearing on upgrade.
</issue_to_address>
### Comment 2
<location path="tests/service/config/test_feature_flags.py" line_range="9" />
<code_context>
+from src.service.config.feature_flags import ENDPOINTS, _flag
+
+
+class TestEndpointFlags:
+ """Tests for the ENDPOINTS dictionary defaults."""
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add explicit tests for default values of each endpoint flag (e.g., fairness/drift True, explainer False)
Current tests only assert that the flags are boolean and that the keys match, but not the specific default values. Given the documented contract (fairness/drift default to True; explainer, explainer_local, explainer_global default to False), please add a test that asserts these values in `ENDPOINTS` when no env overrides are set, so we can detect regressions in default endpoint exposure.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| from src.service.config.feature_flags import ENDPOINTS, _flag | ||
|
|
||
|
|
||
| class TestEndpointFlags: |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add explicit tests for default values of each endpoint flag (e.g., fairness/drift True, explainer False)
Current tests only assert that the flags are boolean and that the keys match, but not the specific default values. Given the documented contract (fairness/drift default to True; explainer, explainer_local, explainer_global default to False), please add a test that asserts these values in ENDPOINTS when no env overrides are set, so we can detect regressions in default endpoint exposure.
![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
🧹 Nitpick comments (4)
src/service/config/feature_flags.py (1)
35-46: 💤 Low value
ENDPOINTSis evaluated once at import time.Because
_flag(...)calls run at module import, changes toTRUSTYAI_ENABLE_*after the process has started have no effect, and tests that need to override flags mustpatch.dictENDPOINTSdirectly (as the registry tests do) rather than mutateos.environ. This is the standard pattern, but worth a one-line note in the module docstring so consumers don't expect runtime reloading.🤖 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 `@src/service/config/feature_flags.py` around lines 35 - 46, ENDPOINTS is populated by calling _flag(...) at import time so any changes to TRUSTYAI_ENABLE_* after startup won't take effect; update the module docstring to add a one-line note that ENDPOINTS (and its _flag evaluations) are evaluated once at import and environment flag changes during runtime will not be picked up, and reference ENDPOINTS, _flag, and the TRUSTYAI_ENABLE_* environment variables so callers/tests know to patch ENDPOINTS directly if they need to override flags at runtime.src/service/config/registry.py (2)
12-13: ⚡ Quick winSilent-default on missing flag keys can hide typos.
_is_enabledreturnsFalsefor any flag not present inENDPOINTS, so a typo in theflag/group_flag/metric_flagargument silently disables the route at startup with no error. Since theENDPOINTSkeys are a fixed, small set, consider raising or at leastlogger.warning(...)when an unknown key is queried, to catch typos early.🛡️ Suggested change
def _is_enabled(flag: str) -> bool: - return ENDPOINTS.get(flag, False) + if flag not in ENDPOINTS: + logger.warning("Unknown feature flag '%s' queried; treating as disabled", flag) + return False + return ENDPOINTS[flag]🤖 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 `@src/service/config/registry.py` around lines 12 - 13, _is_enabled currently returns False for unknown keys which hides typos; update the function (def _is_enabled) to first check if flag is in ENDPOINTS and if not either raise a clear exception (ValueError with the missing flag and list(ENDPOINTS.keys())) to fail fast at startup or at minimum call logger.warning(...) mentioning the unknown flag and available keys before returning False—modify whichever behavior your project prefers and ensure the message includes the flag and ENDPOINTS keys for easy debugging.
16-27: 💤 Low value
# type: ignorecould be avoided by passing kwargs explicitly.The
dict[str, str | list[str]]typing forces a# type: ignore[arg-type]becauseinclude_routerexpects more specific types per kwarg. Callinginclude_routerwith explicit named arguments removes the ignore and is clearer.♻️ Optional refactor
def _include_router( app: FastAPI, router: APIRouter, tag: str | None = None, prefix: str | None = None, ) -> None: - kwargs: dict[str, str | list[str]] = {} - if tag: - kwargs["tags"] = [tag] - if prefix: - kwargs["prefix"] = prefix - app.include_router(router, **kwargs) # type: ignore[arg-type] + if tag and prefix: + app.include_router(router, tags=[tag], prefix=prefix) + elif tag: + app.include_router(router, tags=[tag]) + elif prefix: + app.include_router(router, prefix=prefix) + else: + app.include_router(router)Note: the existing tests assert exact kwargs (
tags=.../prefix=.../none), so this remains compatible.🤖 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 `@src/service/config/registry.py` around lines 16 - 27, The _include_router function uses a generic kwargs dict and then calls app.include_router(... ) with type: ignore; instead, change _include_router to call app.include_router with explicit named args: pass tags=[tag] only when tag is not None and pass prefix=prefix only when prefix is not None (omit each argument when its value is None) so the call signature matches FastAPI's expected types and removes the need for "# type: ignore[arg-type]" (refer to the _include_router function and the app.include_router invocation to locate the change).pyproject.toml (1)
111-111: 💤 Low valuePer-file PLR0913 ignore is reasonable but consider keyword-only params as an alternative.
register_if_enabled_with_grouphas 6 parameters, exceeding the default PLR0913 limit. As an alternative to the per-file ignore, you could mark the optional parameters keyword-only with*,(which improves call-site readability) — though this still wouldn't satisfy PLR0913's count. The current per-file ignore is acceptable; flagging only as a stylistic option.🤖 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 `@pyproject.toml` at line 111, The function register_if_enabled_with_group in registry.py currently has six positional parameters triggering PLR0913; to improve call-site readability, change the signature to make the optional parameters keyword-only by inserting a lone * before the optional args (e.g., def register_if_enabled_with_group(required1, required2, *, optional1=None, optional2=None, ...)), then update all call sites to pass those parameters by name; note this won't reduce the PLR0913 count, so you may keep the existing per-file "PLR0913" ignore if desired.
🤖 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 `@tests/test_app_integration.py`:
- Around line 313-316: The current test collects routes by substring matching
(using "/dir" and "/spd") which is brittle; update the logic in
tests/test_app_integration.py around the fairness_paths computation so it checks
only explicit fairness routes or a strict prefix (e.g., ensure no path starts
with "/fairness/" or compare against a known set of forbidden fairness paths)
instead of using broad substrings; locate the fairness_paths variable and
replace the list comprehension with a filter that either checks
path.startswith("/fairness/") or membership in a defined list of exact paths,
then assert that resulting list is empty.
---
Nitpick comments:
In `@pyproject.toml`:
- Line 111: The function register_if_enabled_with_group in registry.py currently
has six positional parameters triggering PLR0913; to improve call-site
readability, change the signature to make the optional parameters keyword-only
by inserting a lone * before the optional args (e.g., def
register_if_enabled_with_group(required1, required2, *, optional1=None,
optional2=None, ...)), then update all call sites to pass those parameters by
name; note this won't reduce the PLR0913 count, so you may keep the existing
per-file "PLR0913" ignore if desired.
In `@src/service/config/feature_flags.py`:
- Around line 35-46: ENDPOINTS is populated by calling _flag(...) at import time
so any changes to TRUSTYAI_ENABLE_* after startup won't take effect; update the
module docstring to add a one-line note that ENDPOINTS (and its _flag
evaluations) are evaluated once at import and environment flag changes during
runtime will not be picked up, and reference ENDPOINTS, _flag, and the
TRUSTYAI_ENABLE_* environment variables so callers/tests know to patch ENDPOINTS
directly if they need to override flags at runtime.
In `@src/service/config/registry.py`:
- Around line 12-13: _is_enabled currently returns False for unknown keys which
hides typos; update the function (def _is_enabled) to first check if flag is in
ENDPOINTS and if not either raise a clear exception (ValueError with the missing
flag and list(ENDPOINTS.keys())) to fail fast at startup or at minimum call
logger.warning(...) mentioning the unknown flag and available keys before
returning False—modify whichever behavior your project prefers and ensure the
message includes the flag and ENDPOINTS keys for easy debugging.
- Around line 16-27: The _include_router function uses a generic kwargs dict and
then calls app.include_router(... ) with type: ignore; instead, change
_include_router to call app.include_router with explicit named args: pass
tags=[tag] only when tag is not None and pass prefix=prefix only when prefix is
not None (omit each argument when its value is None) so the call signature
matches FastAPI's expected types and removes the need for "# type:
ignore[arg-type]" (refer to the _include_router function and the
app.include_router invocation to locate the change).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01863a9f-4198-4fc7-ad7c-cfdea21324ff
📒 Files selected for processing (9)
pyproject.tomlsrc/main.pysrc/service/config/__init__.pysrc/service/config/feature_flags.pysrc/service/config/registry.pytests/service/config/__init__.pytests/service/config/test_feature_flags.pytests/service/config/test_registry.pytests/test_app_integration.py
Comment on lines
+313
to
+316
| fairness_paths = [ | ||
| p for p in paths if "/fairness/" in p or "/dir" in p or "/spd" in p | ||
| ] | ||
| assert fairness_paths == [] |
There was a problem hiding this comment.
Use explicit fairness path checks instead of broad substring matching.
The "/dir" / "/spd" substring filter is brittle and can accidentally match unrelated future routes. Prefer asserting exact known fairness paths (or a strict fairness prefix) are absent.
Suggested tightening
- fairness_paths = [
- p for p in paths if "/fairness/" in p or "/dir" in p or "/spd" in p
- ]
- assert fairness_paths == []
+ expected_fairness_paths = {
+ "/metrics/fairness/group/dir",
+ "/metrics/fairness/group/dir/definition",
+ "/metrics/fairness/group/dir/request",
+ "/metrics/fairness/group/dir/requests",
+ "/metrics/fairness/group/spd",
+ "/metrics/fairness/group/spd/definition",
+ "/metrics/fairness/group/spd/request",
+ "/metrics/fairness/group/spd/requests",
+ }
+ assert expected_fairness_paths.isdisjoint(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 `@tests/test_app_integration.py` around lines 313 - 316, The current test
collects routes by substring matching (using "/dir" and "/spd") which is
brittle; update the logic in tests/test_app_integration.py around the
fairness_paths computation so it checks only explicit fairness routes or a
strict prefix (e.g., ensure no path starts with "/fairness/" or compare against
a known set of forbidden fairness paths) instead of using broad substrings;
locate the fairness_paths variable and replace the list comprehension with a
filter that either checks path.startswith("/fairness/") or membership in a
defined list of exact paths, then assert that resulting list is empty.
|
PR image build and manifest generation completed successfully! 📦 PR image: 🗂️ CI manifests |
- Log warning on unknown feature flag queries instead of silent default - Refactor _include_router to use explicit args, removing type:ignore - Document explainer default-disabled rationale and import-time eval - Add tests asserting specific default values for each flag group - Tighten fairness path assertions with startswith instead of substring Co-Authored-By: Claude Opus 4.6 <[email protected]>
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
1 participant
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.
Summary
TRUSTYAI_ENABLE_<NAME>environment variablesregister_if_enabled()for single-gate endpoints,register_if_enabled_with_group()for group-gated metricslm_evaluation_harness_routerJira: RHOAIENG-55917
Test plan
uv run pytest tests/service/config/ -v— 15 tests passuv run pytest tests/test_app_integration.py -v— 13 tests passuv run ruff check src tests— lint cleanuv run pyrefly check— type check cleanTRUSTYAI_ENABLE_*env vars🤖 Generated with Claude Code
Summary by Sourcery
Introduce a feature-flag-based system for runtime endpoint gating and apply it to fairness, drift, explainer, and legacy metric routes.
New Features:
Bug Fixes:
Enhancements:
Tests:
Summary by Sourcery
Introduce configurable feature flags to gate runtime registration of metrics and explainer endpoints, and add helpers to centralize router registration based on these flags.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Tests