fix: use session exec in telemetry writer - #13821
Conversation
WalkthroughTelemetry writer flush and retention queries now use ChangesTelemetry writer exec migration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/backend/tests/unit/services/telemetry_writer/test_service.py (1)
347-361: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood deprecation guard; consider one behavioral assertion.
This test only fails if the deprecated
execute()path emits a warning — it asserts nothing about the flush/retention outcome. Since it already drives both code paths against a real in-memory SQLite, a cheap assertion (e.g. the expected post-retention row counts undermax_*_to_keep = 1) would also catch a regression whereexec()silently no-ops or the retention query shape breaks. Note this test also implicitly exercises the vertex-build.scalars()path flagged inservice.py, so confirm it actually passes in CI.🤖 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/services/telemetry_writer/test_service.py` around lines 347 - 361, The deprecation guard in test_flush_and_retention_do_not_call_deprecated_session_execute is good, but the test should also assert the flush/retention result. After calling writer._flush and writer._run_retention_pass, add a behavioral check on the in-memory DB state, such as the expected row counts for transactions and vertex builds when max_transactions_to_keep and max_vertex_builds_to_keep are both 1, so the test catches silent regressions in writer._flush, writer._run_retention_pass, or the vertex-build scalars path.
🤖 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.
Nitpick comments:
In `@src/backend/tests/unit/services/telemetry_writer/test_service.py`:
- Around line 347-361: The deprecation guard in
test_flush_and_retention_do_not_call_deprecated_session_execute is good, but the
test should also assert the flush/retention result. After calling writer._flush
and writer._run_retention_pass, add a behavioral check on the in-memory DB
state, such as the expected row counts for transactions and vertex builds when
max_transactions_to_keep and max_vertex_builds_to_keep are both 1, so the test
catches silent regressions in writer._flush, writer._run_retention_pass, or the
vertex-build scalars path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6ad2ddbd-51b8-4904-b52d-caecd74c6826
📒 Files selected for processing (2)
src/backend/base/langflow/services/telemetry_writer/service.pysrc/backend/tests/unit/services/telemetry_writer/test_service.py
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (54.62%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## release-1.10.2 #13821 +/- ##
==================================================
- Coverage 58.63% 58.57% -0.07%
==================================================
Files 2307 2306 -1
Lines 220990 220169 -821
Branches 34290 32960 -1330
==================================================
- Hits 129574 128956 -618
+ Misses 89933 89729 -204
- Partials 1483 1484 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
* fix: handle invalid session auth tokens (#13823) * fix: handle invalid session auth tokens * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> (cherry picked from commit cf5c37f) * test: cover telemetry writer session warning (#13821) Adapt the warning regression from #13821 for release-1.11.0, which already uses SQLAlchemy AsyncSession via #13845. (cherry picked from commit f5dd643) --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Summary
Tests
Summary by CodeRabbit
Bug Fixes
Tests