fix: add configurable DB_TIMEOUT environment variable for traces API#13555
fix: add configurable DB_TIMEOUT environment variable for traces API#13555TahirMurtaza wants to merge 2 commits into
Conversation
Expose DB_TIMEOUT as an environment variable in the observability settings so operators can tune database query timeouts when paginating traces without requiring a code change. - Added DB_TIMEOUT to observability settings group - Wired timeout into traces.py DB queries - Updated docs (environment-variables.mdx, traces.mdx) - Added unit tests for new setting Fixes langflow-ai#13305
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR makes the hardcoded database timeout for trace queries ( ChangesConfigurable Trace Database Timeout
🎯 2 (Simple) | ⏱️ ~12 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (6 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lfx/tests/unit/services/settings/test_settings_composition.py (1)
102-102: ⚡ Quick winPin the new default value in
test_critical_defaults_unchanged.This set check protects field presence, but not the default contract. Add
assert settings.traces_db_timeout == 5.0in the defaults test so regressions are caught explicitly.As per coding guidelines: “Tests should cover the main functionality being implemented, not just be smoke tests.”
🤖 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/lfx/tests/unit/services/settings/test_settings_composition.py` at line 102, Update the defaults unit test to assert the new default value for traces_db_timeout: inside the test_critical_defaults_unchanged function add an explicit assertion that settings.traces_db_timeout == 5.0 so the test not only verifies the field exists but also pins the default contract; locate the test_critical_defaults_unchanged test and add the assert referencing settings.traces_db_timeout.Source: Coding guidelines
src/backend/tests/unit/api/v1/test_traces_api.py (1)
227-251: ⚡ Quick winAdd the same timeout-propagation assertion for
get_trace.This test covers
/monitor/traces, but the PR also changed/monitor/traces/{trace_id}to use configured timeout. A mirrored test would close the regression gap for the second endpoint.As per coding guidelines: “Verify tests cover both positive and negative scenarios where appropriate” and “Tests should cover the main functionality being implemented.”
🤖 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/backend/tests/unit/api/v1/test_traces_api.py` around lines 227 - 251, Add a mirrored unit test for the single-trace endpoint that asserts the configured DB timeout is used: copy the structure of test_should_use_configured_db_timeout but call the GET for the /monitor/traces/{trace_id} endpoint (e.g., client.get(f"{self._PATH}/some-id") or the equivalent path constant), patch the same symbols (langflow.api.v1.traces.get_settings_service returning the MagicMock settings_service, langflow.api.v1.traces.asyncio.wait_for with the _wait_for side effect, and the single-trace fetch function used by that endpoint—likely langflow.api.v1.traces.fetch_trace—with a small _fetch stub), set settings.traces_db_timeout = 30.0, then assert resp.status_code == 200 and captured_timeouts == [30.0]; name it something like test_should_use_configured_db_timeout_get_trace to mirror the existing test.Source: Coding guidelines
🤖 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 `@src/lfx/src/lfx/services/settings/groups/observability.py`:
- Around line 24-25: The traces_db_timeout setting is not validated and may be
zero or negative, causing immediate timeouts when used with asyncio.wait_for;
update the settings model where traces_db_timeout is declared to enforce
strictly positive values (e.g., add a Field constraint gt=0 or a pydantic
validator on traces_db_timeout) so invalid env values are rejected at load time;
ensure the validation is applied on the same settings class/field named
traces_db_timeout and that error messages clearly state the value must be > 0.
---
Nitpick comments:
In `@src/backend/tests/unit/api/v1/test_traces_api.py`:
- Around line 227-251: Add a mirrored unit test for the single-trace endpoint
that asserts the configured DB timeout is used: copy the structure of
test_should_use_configured_db_timeout but call the GET for the
/monitor/traces/{trace_id} endpoint (e.g., client.get(f"{self._PATH}/some-id")
or the equivalent path constant), patch the same symbols
(langflow.api.v1.traces.get_settings_service returning the MagicMock
settings_service, langflow.api.v1.traces.asyncio.wait_for with the _wait_for
side effect, and the single-trace fetch function used by that endpoint—likely
langflow.api.v1.traces.fetch_trace—with a small _fetch stub), set
settings.traces_db_timeout = 30.0, then assert resp.status_code == 200 and
captured_timeouts == [30.0]; name it something like
test_should_use_configured_db_timeout_get_trace to mirror the existing test.
In `@src/lfx/tests/unit/services/settings/test_settings_composition.py`:
- Line 102: Update the defaults unit test to assert the new default value for
traces_db_timeout: inside the test_critical_defaults_unchanged function add an
explicit assertion that settings.traces_db_timeout == 5.0 so the test not only
verifies the field exists but also pins the default contract; locate the
test_critical_defaults_unchanged test and add the assert referencing
settings.traces_db_timeout.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 75d0fb24-55d2-440a-8db7-c3c02532cdcd
📒 Files selected for processing (6)
docs/docs/Develop/environment-variables.mdxdocs/docs/Develop/traces.mdxsrc/backend/base/langflow/api/v1/traces.pysrc/backend/tests/unit/api/v1/test_traces_api.pysrc/lfx/src/lfx/services/settings/groups/observability.pysrc/lfx/tests/unit/services/settings/test_settings_composition.py
Summary
Fixes #13305
Users were experiencing database timeouts when paginating through traces. This PR exposes a
DB_TIMEOUTenvironment variable so operators can tune the query timeout without a code change.Changes
src/lfx/src/lfx/services/settings/groups/observability.py— AddedDB_TIMEOUTsetting with a sensible defaultsrc/backend/base/langflow/api/v1/traces.py— WiredDB_TIMEOUTinto the traces DB queriesdocs/docs/Develop/environment-variables.mdx— Documented the new variabledocs/docs/Develop/traces.mdx— Added usage note for timeout configurationsrc/backend/tests/unit/api/v1/test_traces_api.py— Unit tests for timeout behavioursrc/lfx/tests/unit/services/settings/test_settings_composition.py— Tests for n## TestingExisting test suite passes
New unit tests cover the default value and env var override
Notes
No breaking changes — the default value preserves existing behaviour.
Summary by CodeRabbit
New Features
LANGFLOW_TRACES_DB_TIMEOUTenvironment variable (default: 5.0 seconds) for improved flexibility on large datasets.Documentation
LANGFLOW_TRACES_DB_TIMEOUTenvironment variable and guidance on adjusting timeouts for large flows.