Update db and rpc metrics to use seconds #618
Conversation
| @@ -9,6 +9,12 @@ release. | |||
|
|
|||
| ### Breaking | |||
|
|
|||
| - Update `db.client.connections.create_time` unit to `s` ([#???](https://github.com/open-telemetry/semantic-conventions/pull/???)) | |||
There was a problem hiding this comment.
you can already update ??? with 618
| @@ -142,7 +142,7 @@ This metric is [recommended][MetricRecommended]. | |||
| <!-- semconv metric.db.client.connections.create_time(metric_table) --> | |||
| | Name | Instrument Type | Unit (UCUM) | Description | | |||
| | -------- | --------------- | ----------- | -------------- | | |||
| | `db.client.connections.create_time` | Histogram | `ms` | The time it took to create a new connection | | |||
| | `db.client.connections.create_time` | Histogram | `s` | The time it took to create a new connection | | |||
There was a problem hiding this comment.
I am aware that the current recommendation is in seconds, but given the minimal time required to establish a connection, possibly in a tiny fraction of a second, should we change it?
There was a problem hiding this comment.
I don't think so. Double floating points have enough precision to capture nano-second precision durations until the durations get quite large. I believe you need to be recording something on the order of a few years in nanoseconds before precision is lost.
Backends can easily translate from seconds to ms, us, or ns without loosing precision so I think its best to conform to the recommendation.
|
There was some discussion about doing this change a while ago. As those metrics are currently in use and the proposed changes would be breaking, the consensus was to do those changes as part of stabilization efforts for DB and RPC semantic conventions and to cover the changes by a compatibility mechanism. The following issues were created for tracking this:
I'd suggest closing this PR for now. Those changes should definitely be done, but as part of overall stabilization efforts. |
| @@ -174,7 +174,7 @@ This metric is [recommended][MetricRecommended]. | |||
| <!-- semconv metric.db.client.connections.use_time(metric_table) --> | |||
| | Name | Instrument Type | Unit (UCUM) | Description | | |||
| | -------- | --------------- | ----------- | -------------- | | |||
| | `db.client.connections.use_time` | Histogram | `ms` | The time between borrowing a connection and returning it to the pool | | |||
| | `db.client.connections.use_time` | Histogram | `s` | The time between borrowing a connection and returning it to the pool | | |||
There was a problem hiding this comment.
shouldn't this also use a different histogram buckets?
|
hi @MadVikingGod! check out #176 (review), and somewhat related open-telemetry/community#1848 |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Fixes use of
mswhen the recommendation iss.Changes
Updated
db.client.connections.create_time,db.client.connections.wait_time,db.client.connections.use_timerpc.server.duration, andrpc.client.durationto use the unitsinstead ofmsMerge requirement checklist