[redisreceiver] Fix duration unit inconsistencies to comply with OTel guidelines #40847
Conversation
…ith OTel guidelines
shmsr
changed the title
|
@dmitryax / @hughesjj / @edmocosta Did you get a chance to look at this? |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Following up on this — let me know if you need anything from my side. |
| recordDataPoint(ts, val) | ||
| if infoKey == "latest_fork_usec" { | ||
| val /= 1e6 // Convert us to s | ||
| recordDataPoint(ts, val) |
There was a problem hiding this comment.
nit: given we're doing recordDataPoint regardless, let's move the call outside of the conditional
| attributes: [db] | ||
| # below are all disabled by default | ||
| redis.replication.replica_offset: | ||
| enabled: false | ||
| description: "Offset for redis replica" | ||
| description: Offset for redis replica |
There was a problem hiding this comment.
nit: could we add the quotes for all these descriptions back, as they're unrelated to the change? Probably better to have quotes regardless when working with yaml, not that we lint it at all.
|
|
||
| | Unit | Metric Type | Value Type | | ||
| | ---- | ----------- | ---------- | | ||
| | us | Gauge | Int | | ||
| | s | Gauge | Double | |
There was a problem hiding this comment.
I'm a wary of changing int into float. Is there any reason we could not floor, truncate, or round-nearest while maintaining an int instead?
@dmitryax I'll defer to you on this one. IIRC there could be some downstream issues in languages using these metrics given the type change.
I'd note the guidelines use SHOULD and not MUST
When instruments are measuring durations, seconds (i.e. s) SHOULD be used.
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Description
Fix unit conversions for duration-based metrics to properly convert from microseconds/milliseconds to seconds
redis.cmd.usectoredis.cmd.secwith proper microsecond to second conversionredis.db.avg_ttlfrom milliseconds to secondsredis.latest_forkfrom microseconds to secondsLink to tracking issue
Fixes #40826
Testing
Documentation
CL added and config.md also updated (not autogenerated)