Review requested:

• @nodejs/sqlite

@araujogui Maybe setSqlTraceHook? This is a bit more explicit and makes it easier to match it up with the corresponding SQLite documentation / functionality.

Codecov Report

❌ Patch coverage is 74.07407% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.67%. Comparing base (da5843b) to head (83627f5).
⚠️ Report is 12 commits behind head on main.

@@            Coverage Diff             @@
##             main   #62241      +/-   ##
==========================================
- Coverage   89.67%   89.67%   -0.01%     
==========================================
  Files         676      676              
  Lines      206469   206582     +113     
  Branches    39537    39562      +25     
==========================================
+ Hits       185157   185244      +87     
- Misses      13448    13465      +17     
- Partials     7864     7873       +9     

Should we make this a diagnostics channel instead?

Should we make this a diagnostics channel instead?

Sounds a good idea to me

CC @nodejs/sqlite @nodejs/diagnostics

Yeah, this seems very much like a thing diagnostics_channel would be more suitable for. There's also a native-side publishing API now, so you can use that.

What is the advantage of using a diagnostic channel here?

What if you have an application with 1000s of DatabaseSync objects and you're only interested in seeing the SQL queries of one particular instance? How would you even differentiate between them? What if you need to do different things with the traces of different database handles (e.g. send them to different logging backends)?

Do diagnostic channels offer that much flexibility?

Note that there are other things that sqlite3_trace_v2 can trace. I think it would be good to eventually support all of them.

The sqlite trace interfaces just gives you some data at the C layer. You still need to figure out how to deliver that efficiently to JS. The diagnostics_channel API is designed to be as close as possible to zero-cost when nothing is listening for a given event, and to be as cheap as possible when something is. It also allows decoupling through named channels so you don't have to explicitly pass around your objects to the things which want to observe them, which makes it much easier for Observability tools to see them. Why reinvent all this machinery to get your data from C to JavaScript when we already have a thing specifically for this purpose?

Because, as I understand it, it is not scoped per database connection. It's all or nothing.

If you have 1000s of concurrent database connections, there's no way to trace only a single of them with this implementation. You will have to deal with a deluge of traces from all of them once you enable the diagnostics channel, with no way to differentiate between messages.