Add time and memory related db operation metrics #1203
Conversation
|
CC @open-telemetry/semconv-db-approvers |
Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
| requirement_level: | ||
| conditionally_required: If available | ||
|
|
||
| - id: metric.db.client.operation.pages_scanned |
There was a problem hiding this comment.
since not all databases use pages, I don't think it makes sense to have this on the "general" view of the database. If there are things that only make sense for a specific group, is worth having those split from here.
There was a problem hiding this comment.
I definitely get the reasoning there and don't necessarily disagree, but if the concept of "pages" (or another example) is shared by multiple databases, either now or in the future, would it make more sense to have a common metric name for them or multiple db-specific names? And could it be more confusing to have some metrics have common names while others would fall under db-specific namespaces? Happy to move these metrics elsewhere if these topics have already been discussed/answered though.
| @@ -33,6 +33,154 @@ groups: | |||
| - ref: db.namespace | |||
| requirement_level: | |||
| conditionally_required: If available. | |||
| - ref: thread.id | |||
There was a problem hiding this comment.
I'm curious about the addition of the thread.id on all of these. Do you have a use case in mind with this?
There was a problem hiding this comment.
My colleague mentioned the rationale in his comment here
In short, for the case of single-threaded operations, the thread id could be useful for troubleshooting slow/blocked queries, and it might also be helpful to aggregate stats on a certain thread for diagnosing performance issues. But we're open to discussion on the attribute and dropping it from the spec if the consensus is that these use cases are too niche and/or not really practical.
There was a problem hiding this comment.
Does thread id add to the cardinality in an uncontrolled way here?
| - ref: db.namespace | ||
| requirement_level: | ||
| conditionally_required: If available. | ||
| - ref: thread.id |
There was a problem hiding this comment.
Nit: maybe say "if available and stable for the lifetime of the operation".
IF using, e.g. async IO and the handler of the response is different than the calling thread - you don't want to create new timeseries because thread.id changes. Can we make that more explicit?
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Fixes #723 (split off from #1186)
Changes
Adding additional db operation metrics related to memory and time spent.
Merge requirement checklist
[chore]