Skip to content

Replace print() error output with structured logging (#68)#77

Open
bradjin8 wants to merge 8 commits into
masterfrom
feat/replace-print-with-logging
Open

Replace print() error output with structured logging (#68)#77
bradjin8 wants to merge 8 commits into
masterfrom
feat/replace-print-with-logging

Conversation

@bradjin8
Copy link
Copy Markdown
Collaborator

@bradjin8 bradjin8 commented May 25, 2026

Summary

  • Configure default application logging in create_app() (logging.basicConfig at INFO, format includes module and function name for gunicorn/WSGI capture).
  • Replace all error-reporting print() calls in services/ and api/ with module loggers (logging.getLogger(__name__)) using warning for schema drift and error with exc_info=True for unexpected failures.
  • Remove traceback.print_exc() from export/PDF handlers in favor of structured traceback via exc_info=True.
  • Update test_bubble_schema_drift_is_logged_not_swallowed_silently to assert on log output (assertLogs) instead of stdout.

Closes #68. Complements #66 (except-pass → logging): services that already logged parse failures are unchanged; this PR finishes Test 10 coverage for paths that still used print().

Files touched: app.py, services/cli_tabs.py, api/composers.py, api/search.py, api/export_api.py, api/pdf.py, api/config_api.py, tests/test_models_wired_at_read_sites.py

Out of scope: scripts/export.py CLI stderr warnings (not under services/ / api/ per issue).

Test plan

  • python -m pytest -q — 306 passed, 4 skipped
  • No print( calls remain in services/ or api/
  • Manual: start app locally, trigger a schema-drift row (or bad CLI session path) and confirm WARNING/ERROR lines appear in server logs with module name and entity id
  • Reviewer: confirm log format is acceptable behind gunicorn per DEPLOYMENT.md

Summary by CodeRabbit

  • Bug Fixes

    • Improved error handling and structured logging across the app so parsing/schema failures and other errors are logged (with diagnostics) while problematic entries are skipped without disrupting user flows.
  • Chores

    • Updated Click dependency to version 8.4.1.
  • Tests

    • Added and adjusted tests validating resilient behavior and that warnings are emitted for parse/schema issues.

Review Change Stack

@bradjin8 bradjin8 self-assigned this May 25, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 25, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 396c6bb6-8300-45d7-b5ed-67ff5b3531a4

📥 Commits

Reviewing files that changed from the base of the PR and between 048686a and 7238424.

📒 Files selected for processing (1)
  • services/workspace_tabs.py

📝 Walkthrough

Walkthrough

Replace print/silent-except error paths with module-level logging, add logging.basicConfig in app factory, convert prints to _logger.warning/_logger.error (use exc_info where used), and add tests that assert emitted warnings for parse/schema-drift failures.

Changes

Structured Logging Across API and Service Layers

Layer / File(s) Summary
Logging infrastructure and app setup
app.py
Configure global logging via logging.basicConfig(...) and import logging in the app factory.
API: Composer and search schema-drift logging
api/composers.py, api/search.py
Replace print-based schema-drift/error reporting with module-level _logger.warning/_logger.error calls in list_composers, get_composer, and search parsing paths.
API: Config, export, PDF, logs, and workspaces logging
api/config_api.py, api/export_api.py, api/pdf.py, api/logs.py, api/workspaces.py
Add _logger and convert exception handlers and formerly-silent excepts/prints to structured _logger.warning/_logger.error (include exc_info where used).
Services: Workspace tabs parsing and logging
services/workspace_tabs.py
Add _logger and _kv_payload_log_meta; explicitly skip NULL cursorDiskKV rows and log JSON-decode and schema-drift failures instead of printing or silently ignoring.
Services: Workspace listing, resolver, and CLI tabs logging
services/workspace_listing.py, services/workspace_resolver.py, services/cli_tabs.py
Introduce module loggers and convert print()/bare excepts to structured warnings/errors for workspace.json reads, Composer/Bubble decoding, mtime resolution, and CLI session processing.
Tests: Logging assertions and parse-failure coverage
tests/test_parse_failure_logging.py, tests/test_models_wired_at_read_sites.py, tests/test_invalid_workspace_aliases.py, tests/test_workspace_tabs_null_bubble.py
New tests that seed drifted/invalid DB rows and assert WARNING logs; updated existing tests to use assertLogs and check warning messages.
Dependency: Click version update
requirements-lock.txt
Bump click lock from 8.4.0 to 8.4.1.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers

  • timon0305
  • wpak-ai

"I hopped through code and found a print,
I nudged it into logs so warnings glint.
Silent fails now sing their name,
Stack traces help to chase the blame.
— Rabbit, logging engineer 🐇"

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Replace print() error output with structured logging (#68)' accurately and clearly summarizes the main objective of the pull request.
Linked Issues check ✅ Passed All acceptance criteria from issue #68 are met: print() calls replaced with logging, loggers added to all service modules, context included in messages, logging.basicConfig configured in app.py, tests pass, and implementation follows guidelines.
Out of Scope Changes check ✅ Passed All changes are directly scoped to replacing print() with structured logging in services/ and api/ directories as specified in issue #68; no unrelated modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/replace-print-with-logging

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 `@services/workspace_listing.py`:
- Line 137: The top-level composer load error log currently drops stack context
by calling _logger.error("Failed to load composer rows from global storage: %s",
e); update that call to include the exception traceback by passing exc_info=True
(i.e., _logger.error(..., exc_info=True)) so the stack trace is preserved for
operational debugging while keeping the existing message and exception
interpolation.

In `@services/workspace_tabs.py`:
- Around line 126-131: The warning currently logs the raw cursorDiskKV payload
(row["value"]) which can leak sensitive conversation data and be very large;
update the _logger.warning call(s) that reference row["value"] (the one shown
and the similar block around lines 210-215) to avoid including the raw payload
and instead log stable identifiers and payload size — keep row["key"] (or any
available stable id like bubble id) and add payload length (e.g.,
len(row.get("value", b"")) or length of the string) and/or a checksum/short hash
if you need a stable fingerprint; update the _logger.warning invocations in
workspace_tabs.py that currently format row["value"] accordingly.
🪄 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: aced6938-416d-42b6-b9e3-5c4e0857d0f0

📥 Commits

Reviewing files that changed from the base of the PR and between 731c1e1 and 2a7c44e.

📒 Files selected for processing (17)
  • api/composers.py
  • api/config_api.py
  • api/export_api.py
  • api/logs.py
  • api/pdf.py
  • api/search.py
  • api/workspaces.py
  • app.py
  • requirements-lock.txt
  • services/cli_tabs.py
  • services/workspace_listing.py
  • services/workspace_resolver.py
  • services/workspace_tabs.py
  • tests/test_invalid_workspace_aliases.py
  • tests/test_models_wired_at_read_sites.py
  • tests/test_parse_failure_logging.py
  • tests/test_workspace_tabs_null_bubble.py

Comment thread services/workspace_listing.py Outdated
Comment thread services/workspace_tabs.py
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@services/workspace_tabs.py`:
- Around line 138-145: The JSON decode guard around json.loads in
services/workspace_tabs.py currently only catches json.JSONDecodeError; change
those except clauses (the one around the block that calls _kv_payload_log_meta
and logs via _logger.warning with row["key"] and e) to catch
(json.JSONDecodeError, TypeError, ValueError) so non-text SQLite values or other
decode issues are treated non-fatal; apply the same change to the other
symmetric decode block that logs payload_len/payload_fp (the second handler that
currently only catches JSONDecodeError) so both malformed KV row paths behave
consistently.
🪄 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: 65d3d309-c7f4-4f28-8a15-8bc988cd7bf5

📥 Commits

Reviewing files that changed from the base of the PR and between 2a7c44e and 048686a.

📒 Files selected for processing (2)
  • services/workspace_listing.py
  • services/workspace_tabs.py

Comment thread services/workspace_tabs.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace print() error output with structured logging

1 participant