feat: add startup and document save failure logs for improved debugging#46
feat: add startup and document save failure logs for improved debugging#46anshuman9468 wants to merge 1 commit intodbpedia:masterfrom
Conversation
📝 WalkthroughWalkthroughThis PR adds observability through SLF4J logging to three core components: ApiImpl logs file commit failures, JettyLauncher reports git directory configuration status, and SparqlClient logs RDF parsing errors. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~4 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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.
🧹 Nitpick comments (3)
src/main/scala/org/dbpedia/databus/ApiImpl.scala (1)
173-177: Drop the defensive innertry-catcharoundlog.error— SLF4J does not throw.SLF4J's contract guarantees that logging calls do not propagate exceptions. Wrapping
log.error(...)in a separatetry-catchadds noise without providing any real protection.♻️ Proposed fix
.recoverWith { case err: Throwable => - try { - log.error(s"Failed to save files for user '$username' (${fullFilenamesAndData.keys.mkString(",")})", err) - } catch { - case _: Throwable => // ignore logging failures - } + log.error(s"Failed to save files for user '$username' (${fullFilenamesAndData.keys.mkString(",")})", err) Failure(err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/scala/org/dbpedia/databus/ApiImpl.scala` around lines 173 - 177, Remove the redundant defensive inner try-catch around the logging call in ApiImpl.scala: instead of wrapping log.error(s"Failed to save files for user '$username' (${fullFilenamesAndData.keys.mkString(",")})", err) in a try { ... } catch { case _: Throwable => ... }, just call log.error(...) directly (no surrounding try-catch). This removes noise because SLF4J guarantees logging calls won't throw; keep the existing error message, variables (username, fullFilenamesAndData) and the err parameter unchanged.src/main/scala/org/dbpedia/databus/JettyLauncher.scala (1)
42-47: ReplaceisDefined + .getwith idiomatic pattern matching.Calling
.geton anOption— even when guarded byisDefined— is non-idiomatic in Scala and suppresses the safe-access benefit ofOption. Amatchexpression avoids.getentirely.♻️ Proposed fix
- if (config.gitLocalDir.isDefined) { - log.info(s"Using local git directory: ${config.gitLocalDir.get.toAbsolutePath}") - log.info("Attempting to cache local development context (if present)") - } else { - log.info("No local git directory configured; using remote git API (if configured)") - } + config.gitLocalDir match { + case Some(dir) => + log.info(s"Using local git directory: ${dir.toAbsolutePath}") + log.info("Attempting to cache local development context (if present)") + case None => + log.info("No local git directory configured; using remote git API (if configured)") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/scala/org/dbpedia/databus/JettyLauncher.scala` around lines 42 - 47, Replace the isDefined + .get pattern on config.gitLocalDir with safe Option handling: match on config.gitLocalDir (or use foreach/fold) and call log.info with the path inside the Some(path) branch, otherwise use the existing else message; update the block containing config.gitLocalDir, the two log.info calls and remove .get to avoid unsafe access.src/main/scala/org/dbpedia/databus/SparqlClient.scala (1)
160-162: Pass theThrowableas the second argument tolog.errorto capture the stack trace.The current call constructs a string with
e.getMessage, which only logs the exception message — losing the full stack trace. SLF4J recognises a trailingThrowableargument and appends the complete stack trace, which is far more useful for debugging. Note thate.getMessagecan also benullfor someThrowablesubclasses, producing the literal string"null"in the log. Both issues are fixed by using the two-argument form, consistent withApiImpl.scalaLine 174.♻️ Proposed fix
- log.error(s"Failed to parse input as ${lang.getName}: ${e.getMessage}") + log.error(s"Failed to parse input as ${lang.getName}", e)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/scala/org/dbpedia/databus/SparqlClient.scala` around lines 160 - 162, The log call in the exception handler inside SparqlClient.scala's parsing block (the `case e: Throwable` branch) only interpolates e.getMessage and thus loses the stack trace; change the call to pass the Throwable as the second argument to SLF4J (use the two-argument form like log.error("Failed to parse input as {}: {}", lang.getName, e.getMessage, e) or equivalent) so the full stack trace is recorded—update the `case e: Throwable` handler accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/scala/org/dbpedia/databus/ApiImpl.scala`:
- Around line 173-177: Remove the redundant defensive inner try-catch around the
logging call in ApiImpl.scala: instead of wrapping log.error(s"Failed to save
files for user '$username' (${fullFilenamesAndData.keys.mkString(",")})", err)
in a try { ... } catch { case _: Throwable => ... }, just call log.error(...)
directly (no surrounding try-catch). This removes noise because SLF4J guarantees
logging calls won't throw; keep the existing error message, variables (username,
fullFilenamesAndData) and the err parameter unchanged.
In `@src/main/scala/org/dbpedia/databus/JettyLauncher.scala`:
- Around line 42-47: Replace the isDefined + .get pattern on config.gitLocalDir
with safe Option handling: match on config.gitLocalDir (or use foreach/fold) and
call log.info with the path inside the Some(path) branch, otherwise use the
existing else message; update the block containing config.gitLocalDir, the two
log.info calls and remove .get to avoid unsafe access.
In `@src/main/scala/org/dbpedia/databus/SparqlClient.scala`:
- Around line 160-162: The log call in the exception handler inside
SparqlClient.scala's parsing block (the `case e: Throwable` branch) only
interpolates e.getMessage and thus loses the stack trace; change the call to
pass the Throwable as the second argument to SLF4J (use the two-argument form
like log.error("Failed to parse input as {}: {}", lang.getName, e.getMessage, e)
or equivalent) so the full stack trace is recorded—update the `case e:
Throwable` handler accordingly.
Adds console logs during startup (local dev context caching) and when document save operations fail, particularly for remote context loading errors.
This improves debugging visibility and speeds up issue identification.
Summary by CodeRabbit