Send SQLite errors to DO sentry#6563
Conversation
a161b20 to
215ab85
Compare
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This adds a SENTRY_DO prefix to the SQLITE_REQUIRE macro's KJ_FAIL_REQUIRE message to route SQLite errors to DO Sentry.
Issues (by severity):
-
[MEDIUM]
SENTRY_DOtag applied too broadly. TheSQLITE_REQUIREmacro is defined inutil/sqlite.c++, which is a general-purpose SQLite wrapper. It is used not only for Durable Object storage (ActorSqlite,SqlStorage,SqliteKv) but also byAlarmScheduler(server-level alarm persistence inserver/alarm-scheduler.c++) and genericSqliteDatabaseoperations with theTRUSTEDregulator. Tagging allSQLITE_REQUIREfailures asSENTRY_DOwould misattribute non-DO SQLite errors. In practice this may be acceptable if the AlarmScheduler never hits these paths in production, but it's worth noting. -
[LOW] Potential conflict with
NOSENTRYonSQLITE_BUSY. Line 290 passeskj::str("NOSENTRY ", dbErrorMessage(error, db))as theerrorMessagetoSQLITE_REQUIRE. After this change,KJ_FAIL_REQUIREproduces a description of"SENTRY_DO SQLite failed"with"NOSENTRY ..."in the detail arguments. Whether theSENTRY_DOin the description overrides theNOSENTRYin the detail depends on how the internal log pipeline parses these -- worth verifying thatSQLITE_BUSYerrors don't start showing up in DO Sentry unexpectedly.
This review was generated by an AI assistant and may contain inaccuracies.
|
I've posted my review on PR #6563. Here's a summary of the findings: What the PR does: Adds a Issues found:
Neither issue has a concrete code fix to suggest (they're design/architecture questions requiring author input), so no suggestion comments were posted. |
No description provided.