-
-
Notifications
You must be signed in to change notification settings - Fork 35.3k
sqlite: add diagnostic channel #62241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 8 commits
2ac7a27
349412e
69e5438
27985a1
3c36722
8bfc4fd
c40646d
906ef83
2d36987
89ab713
24f6ea0
9908e0b
4b20c09
dacc3c7
872bc6d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| 'use strict'; | ||
| const common = require('../common.js'); | ||
| const sqlite = require('node:sqlite'); | ||
| const dc = require('diagnostics_channel'); | ||
| const assert = require('assert'); | ||
|
|
||
| const bench = common.createBenchmark(main, { | ||
| n: [1e5], | ||
| mode: ['none', 'subscribed', 'unsubscribed'], | ||
| }); | ||
|
|
||
| function main(conf) { | ||
| const { n, mode } = conf; | ||
|
|
||
| const db = new sqlite.DatabaseSync(':memory:'); | ||
| db.exec('CREATE TABLE t (x INTEGER)'); | ||
| const insert = db.prepare('INSERT INTO t VALUES (?)'); | ||
|
|
||
| let subscriber; | ||
| if (mode === 'subscribed') { | ||
| subscriber = () => {}; | ||
| dc.subscribe('sqlite.db.query', subscriber); | ||
| } else if (mode === 'unsubscribed') { | ||
| subscriber = () => {}; | ||
| dc.subscribe('sqlite.db.query', subscriber); | ||
| dc.unsubscribe('sqlite.db.query', subscriber); | ||
| } | ||
| // mode === 'none': no subscription ever made | ||
|
|
||
| let result; | ||
| bench.start(); | ||
| for (let i = 0; i < n; i++) { | ||
| result = insert.run(i); | ||
| } | ||
| bench.end(n); | ||
|
|
||
| if (mode === 'subscribed') { | ||
| dc.unsubscribe('sqlite.db.query', subscriber); | ||
| } | ||
|
|
||
| assert.ok(result !== undefined); | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,7 +5,9 @@ | |||||||||
| #include "env-inl.h" | ||||||||||
| #include "memory_tracker-inl.h" | ||||||||||
| #include "node.h" | ||||||||||
| #include "node_diagnostics_channel.h" | ||||||||||
| #include "node_errors.h" | ||||||||||
| #include "node_external_reference.h" | ||||||||||
| #include "node_mem-inl.h" | ||||||||||
| #include "node_url.h" | ||||||||||
| #include "sqlite3.h" | ||||||||||
|
|
@@ -63,6 +65,28 @@ using v8::TryCatch; | |||||||||
| using v8::Uint8Array; | ||||||||||
| using v8::Value; | ||||||||||
|
|
||||||||||
| BindingData::BindingData(Realm* realm, Local<Object> wrap) | ||||||||||
| : BaseObject(realm, wrap) { | ||||||||||
| MakeWeak(); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| void BindingData::MemoryInfo(MemoryTracker* tracker) const { | ||||||||||
| tracker->TrackFieldWithSize("open_databases", | ||||||||||
| open_databases.size() * sizeof(DatabaseSync*), | ||||||||||
| "open_databases"); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| void BindingData::CreatePerContextProperties(Local<Object> target, | ||||||||||
| Local<Value> unused, | ||||||||||
| Local<Context> context, | ||||||||||
| void* priv) { | ||||||||||
| Realm* realm = Realm::GetCurrent(context); | ||||||||||
| realm->AddBindingData<BindingData>(target); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| void BindingData::RegisterExternalReferences( | ||||||||||
| ExternalReferenceRegistry* registry) {} | ||||||||||
|
||||||||||
| ExternalReferenceRegistry* registry) {} | |
| ExternalReferenceRegistry* registry) { | |
| registry->Register(CreatePerContextProperties); | |
| } |
Copilot
AI
Apr 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DatabaseSync registers itself in env->principal_realm()->GetBindingData<BindingData>(), but BindingData::CreatePerContextProperties() currently installs BindingData on the current Realm. If node:sqlite is loaded from a non-principal Realm (e.g., vm/ShadowRealm), principal_realm() may not have this BindingData, so open_databases won’t be tracked and trace enable/disable via diagnostics_channel will not work. Consider making sqlite BindingData truly per-Environment (always stored on env->principal_realm()), or consistently use the current Realm for both creation and lookup.
| BindingData* binding = env->principal_realm()->GetBindingData<BindingData>(); | |
| BindingData* binding = env->context()->GetBindingData<BindingData>(); |
Renegade334 marked this conversation as resolved.
Show resolved
Hide resolved
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to get the query template and values as they were prior to generating the final SQL string? If at all possible, I think it would be valuable for Observability purposes to publish an event which includes: database instance, query template, query parameters, and final query string.
Uh oh!
There was an error while loading. Please reload this page.