-
Notifications
You must be signed in to change notification settings - Fork 162
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
Add time and memory related db operation metrics #1203
base: main
Are you sure you want to change the base?
Add time and memory related db operation metrics #1203
Conversation
CC @open-telemetry/semconv-db-approvers |
requirement_level: | ||
conditionally_required: If available | ||
|
||
- id: metric.db.client.operation.pages_scanned |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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.
Good call, done
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]