Conversation
There was a problem hiding this comment.
Thank you for the PR @shyim. This is a good start!
I'd like us to follow the structure of https://.com/getsentry/sentry-javascript/blob/develop/packages/cloudflare/src/d1.ts more closely here.
That means we need to
- Make sure we create breadcrumbs for SQL queries
- Link to bun docs wherever appropriate
- Extract out functions for
createStartSpanOptionsandcreateSpanAttributes
I understand thats a lot though (plus all of my review comments). Maybe we can start by creating a simple version of bunSqliteIntegration that just es one method. Then we can implement the rest of the ing in a follow-up PR.
| let hasedBunSqlite = false; | ||
| export function _resetBunSqliteInstrumentation(): void { | ||
| hasedBunSqlite = false; |
There was a problem hiding this comment.
We can remove this functionality, it doesn't actually reset the proxy implementation, so calling this will lead to the sql module getting ed multiple times.
| construct(target, args) { | ||
| const instance = new target(...args); | ||
| if (args[0]) { | ||
| Object.defineProperty(instance, '_sentryDbName', { |
There was a problem hiding this comment.
Where is this being set?
It only seems to be defined, which would mean let dbName = this._sentryDbName || dbNameMap.get(this); would always evaluate dbNameMap.get(this)
| op: `db.sql.statement.${method}`, | ||
| attributes: { | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.db.bun.sqlite', | ||
| 'db.system': 'sqlite', |
There was a problem hiding this comment.
| 'db.system': 'sqlite', | |
| 'db.system.name': 'sqlite', |
https://opentelemetry.io/docs/specs/semconv/registry/attributes/db/#db-system
| const inParentSpanMap = new WeakMap<any, boolean>(); | ||
| const dbNameMap = new WeakMap<any, string>(); |
There was a problem hiding this comment.
What do these maps do? Can we type them more stronger? Why are they WeakMap?
| let dbName = this._sentryDbName || dbNameMap.get(this); | ||
| if (!dbName && this.filename) { | ||
| dbName = this.filename; | ||
| dbNameMap.set(this, dbName); | ||
| } |
| return startSpan( | ||
| { | ||
| name: sql || 'db.sql.' + method, |
There was a problem hiding this comment.
In what cases is sql not defined? We should always try to make the span name the db statement.
| op: `db.sql.${method}`, | ||
| attributes: { | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.db.bun.sqlite', | ||
| 'db.system': 'sqlite', |
There was a problem hiding this comment.
| 'db.system': 'sqlite', | |
| 'db.system.name': 'sqlite', |
https://opentelemetry.io/docs/specs/semconv/registry/attributes/db/#db-system
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.db.bun.sqlite', | ||
| 'db.system': 'sqlite', | ||
| 'db.operation': method, | ||
| ...(sql && { 'db.statement': sql }), |
There was a problem hiding this comment.
| ...(sql && { 'db.statement': sql }), | |
| ...(sql && { 'db.query.text': sql }), |
https://opentelemetry.io/docs/specs/semconv/registry/attributes/db/#db-statement
| 'db.system': 'sqlite', | ||
| 'db.operation': method, | ||
| ...(sql && { 'db.statement': sql }), | ||
| ...(dbName && { 'db.name': dbName }), |
There was a problem hiding this comment.
| ...(dbName && { 'db.name': dbName }), | |
| ...(dbName && { 'db.namespace': dbName }), |
https://opentelemetry.io/docs/specs/semconv/registry/attributes/db/#db-name
| return result; | ||
| } catch (error) { | ||
| span.setStatus({ code: 2, message: 'internal_error' }); | ||
| captureException(error, { |
There was a problem hiding this comment.
We don't need to capture an error here, we can just let the error bubble up.
Adds auto instrumentation support for Bun Sqlite
yarn lint) & (yarn test).