-
Notifications
You must be signed in to change notification settings - Fork 461
[Instrumentation] D1 follow ups #5373
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
Conversation
| name: 'd1_batch', | ||
| 'db.system.name': 'cloudflare-d1', | ||
| 'db.operation.name': 'batch', | ||
| 'db.query.text': | ||
| 'SELECT * FROM users WHERE user_id = ?;\n' + | ||
| 'SELECT * FROM users WHERE user_id = ?;', | ||
| 'db.operation.batch.size': 2, | ||
| 'cloudflare.binding.type': 'D1', | ||
| 'cloudflare.d1.response.size_after': 8192, | ||
| 'cloudflare.d1.response.rows_read': 2, | ||
| 'cloudflare.d1.response.rows_written': 0, | ||
| 'cloudflare.d1.response.last_row_id': 2, | ||
| 'cloudflare.d1.response.changed_db': false, | ||
| 'cloudflare.d1.response.changes': 0, | ||
| 'cloudflare.d1.response.queries_count': 2, | ||
| closed: true, |
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.
@lambrospetrou This is an example of a batch span. All of the queries are visible here, and I've added a count.
My understanding based on #5218 (comment) is that what you would like to see is more like:
{
name: 'd1_batch',
'db.system.name': 'cloudflare-d1',
'db.operation.name': 'batch',
'db.query.text':
'SELECT * FROM users WHERE user_id = ?;\n' +
'SELECT * FROM users WHERE user_id = ?;',
'db.operation.batch.size': 2,
'cloudflare.binding.type': 'D1',
'cloudflare.d1.response.size_after': 8192,
'cloudflare.d1.response.rows_read': 2,
'cloudflare.d1.response.rows_written': 0,
'cloudflare.d1.response.last_row_id': 2,
'cloudflare.d1.response.changed_db': false,
'cloudflare.d1.response.changes': 0,
'cloudflare.d1.response.queries_count': 2,
'cloudflare.d1.response.meta':
'[{"duration":0.0014168027188820064,"served_by":"d1-mock","changes":0,"last_row_id":2,"changed_db":false,"size_after":8192,"rows_read":1,"rows_written":0},{"duration":0.005165785717162432,"served_by":"d1-mock","changes":0,"last_row_id":2,"changed_db":false,"size_after":8192,"rows_read":1,"rows_written":0}]',
closed: true,
}or that but broken out into more fields?
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.
@lambrospetrou After thinking about it, I think the answer here is likely attaching these as span events. For each query in the batch emit a span event with the query, and all the meta data. That would keep it all queryable and clear.
However we don't yet have the ability to emit span events 😅 Mind if I file a ticket and come back to this?
src/cloudflare/internal/d1-api.ts
Outdated
| function addAggregatedD1MetaToSpan(span: Span, metas: D1Meta[]): void { | ||
| const aggregatedMeta = aggregateD1Meta(metas); | ||
| addD1MetaToSpan(span, aggregatedMeta); | ||
| span.setAttribute('cloudflare.d1.response.queries_count', metas.length); |
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.
This isn't necessary since we already had this data available under the semantic convention name: db.operation.batch.size: https://github.com/cloudflare/workerd/blob/main/src/cloudflare/internal/d1-api.ts#L207
|
The generated output of Full Type Diff |
|
I've tried a couple of things, but haven't been able to find a way to import the |
No description provided.