Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Database metric semantic conventions #1076
Database metric semantic conventions #1076
Changes from 19 commits
9196ab8
330e728
4fbd26f
2ae2bbb
1c2b9e9
c220dfd
9924cff
6b66fd3
10a3ef4
e9b0bdb
506fe6e
b16d67d
deea045
4d96aab
dc92381
88c6e1c
7bdc85d
cf83046
ec58de4
9f95457
b2bebfc
e6acc6a
efba901
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 these are in a different order than https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/database.md#connection-level-attributes (maybe the tracing spec was updated since)
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.
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.
Seems like this is supposed to pertain to a
db.statement
parameter, which is missing in the table.Might also be worth adding "it is not recommended to attempt any client-side parsing of
db.statement
to remove parameters", although users shouldn't be adding formatting them in anyway. The trace conventions say "the value may be sanitized to exclude sensitive information," do you think that is mostly just about not substitution values?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 like this. I've updated it to be more like these words. What do you think?
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.
The
db.operation
label wouldn't have any placeholders I thought, since its justSELECT
,UPDATE
, etc. I was more thinking that advice would apply todb.statement
, unless it was purposefully left out for cardinality reasons?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.
Without
db.statement
, is there enough granularity? In the MySQL example, allSELECT
queries toorders
would be aggregated into that same metricThere 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.
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.
These remind me of the MemStats variables, which are difficult to reason about. This makes me think they should be database-specific, e.g.,
db.redis.connections.taken
, maybe?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.
OK, I think I'm on board. As I said before, I don't have strong use cases for these metrics, aside from the fact that go-redis is already collecting them.
My only hesitation is: if other database libraries decide to pick up these metrics, and they become somewhat common, I wonder if it diminishes their value to have them namespaced separately.
As a user, If I have redis and postgres connections, will I be frustrated that they report separately as
db.redis.connections.taken
anddb.postgres.connections.taken
?Now that I've asked that, though, it doesn't seem very useful to aggregate connection pool information across multiple databases, so maybe it's fine to separate them.
The other option (that I'd be definitely in favor of) is to just drop these metrics entirely, so we'd only specify
db.connection_pool.limit
anddb.connection_pool.usage
.Which of those would you prefer?
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.
From the metrics SIG today: we're going to give guidance to use
db.{db.system}.*
for technology-specific instrument names.And I'll include some examples using the names from go-redis.