Fix/log swallowed exceptions#263
Conversation
📝 WalkthroughWalkthroughTwo bug fixes improve error visibility and concurrency: silent exceptions in graph initialization are now logged with descriptive messages, and embedding operations are refactored to execute synchronously in separate threads via asyncio to prevent blocking. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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 |
|
@smokeyScraper please review it once |
Shevilll
left a comment
There was a problem hiding this comment.
Hello @dolliecoder! 👋
This is a fantastic contribution! Replacing silent except Exception: pass blocks with proper logging is a huge step forward for the observability and maintainability of the project, and offloading the CPU-heavy sentence transformer embedding encoding to a separate thread via asyncio.to_thread is an excellent, standard Python concurrency optimization.
Here are a few expert technical notes and design suggestions for this pull request:
🌟 What makes this PR great:
- Event Loop Non-Blocking Optimization: Offloading
SentenceTransformer.encodeto a worker thread usingasyncio.to_threadis exactly the right pattern in FastAPI. This ensures concurrent API requests aren't starved while computing high-dimension vector embeddings. Great job! - Eliminating Silent Failures: Exposing previously swallowed exceptions makes debugging development setups immensely easier.
💡 Crucial Best Practice Suggestions:
-
Handling Expected "Index Already Exists" Errors in FalkorDB:
- Observation: In
graph.py(lines 14–31), when FalkorDB initializes, it attempts to create indexes. If the index was already created during a previous run, FalkorDB'screate_node_range_indexandcreate_node_fulltext_indexwill raise an exception. - Problem: Because this exception is completely expected and normal on every startup after the first one, using
logger.exception()(which logs at the ERROR level with a full stack traceback) will flood the production logs with tracebacks during normal operation. This can lead to false alarms in error tracking software like Sentry or Datadog. - Recommendation: Instead of logging a full traceback at the ERROR level, we can catch the exception and log a simple, friendly info or debug message:
try: self.g.create_node_range_index("File", "name", "ext") except Exception as e: # Log at INFO/DEBUG level since the index likely already exists logger.info("Note: File range index creation skipped or already exists: %s", str(e))
- Observation: In
-
Python Style & PEP 8:
- The type annotations look clean, and importing
asynciois correctly done. - There are a couple of extra double-blank lines introduced in
service.py(around lines 67 and 84), which can be quickly cleaned up to keep the formatting extra tidy.
- The type annotations look clean, and importing
This is a highly impactful PR for both performance and debugging! Thank you for your hard work and contribution to GSoC! 🚀
Shevilll
left a comment
There was a problem hiding this comment.
Peer Review: Fix/log swallowed exceptions
Hello @dolliecoder! Thank you for addressing the swallowed exceptions issue. Improving observability and logging quality in initialization sequences is extremely helpful for debugging.
I have a feedback item regarding potential false-alarm logs and log flooding:
⚠️ Observation: Log Flooding on Existing Indices
In backend/app/database/falkor/code-graph-backend/api/graph.py, you replaced except Exception: pass with logger.exception(...) during node range and full-text index creation:
try:
self.g.create_node_range_index("File", "name", "ext")
except Exception:
logger.exception(
"Failed to create node range index for File(name, ext)"
)The Issue:
In FalkorDB / RedisGraph, attempting to create an index that already exists will raise an exception (e.g. Index already exists).
Because graph initialization runs every time the application starts up or when a new graph context is established, this means every subsequent run of the server will log a massive ERROR log with a full traceback/stack trace simply because the indices were already created in a previous session.
This leads to:
- Log pollution: Cluttering standard error output and log-aggregation systems with false alarms.
- Alert fatigue: Devs/SREs might see
ERRORwith traceback and waste time investigating a non-issue.
💡 Suggested Fix
We can selectively silence or log a brief info/debug message if the exception is due to the index already existing, while still logging full tracebacks for unexpected actual failures.
For example, we can check the exception message:
try:
self.g.create_node_range_index("File", "name", "ext")
except Exception as e:
if "already exists" in str(e).lower():
logger.info("Node range index for File(name, ext) already exists.")
else:
logger.exception(
"Unexpected failure creating node range index for File(name, ext)"
)This keeps the logs clean during normal operations (where the index already exists) while fully preserving the observability benefits of your PR for genuine failures (e.g., connection drops, permission issues, or syntactical errors).
What are your thoughts on this approach? Thanks again for your work!
fixes #259
Replaced
except Exception: passblocks withlogger.exception().Previously, initialization failures were silently ignored, making debugging difficult.
Now errors are logged with stack traces for better observability.
No behavior changes, only improved logging.
Summary by CodeRabbit
Performance
Chores