-
Notifications
You must be signed in to change notification settings - Fork 3.1k
redisreceiver: add additional metrics #39860
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
redisreceiver: add additional metrics #39860
Conversation
This commit adds metrics to the redisreceiver that are present in the telegraf collector. Improving parity between the telegraf collector and the redisreceiver can help users that are moving between the otel collector and the telegraf collector. Fixes open-telemetry#39859 Signed-off-by: Eric Chlebek <[email protected]>
4adb0ea
to
8c7c42e
Compare
Signed-off-by: Eric Chlebek <[email protected]>
Please add a changelog. Please follow the guidance in the docs I gave you. Your PR lacks the code to actually capture those metrics. Crucially, please check that none of the metrics you added are not already collected or cannot be computed from existing metrics by combining them. |
@atoulme Will do, thank you. |
Moving to draft while you address this feedback. Please mark ready for review once CI passes and the review comments are addressed. |
This metrics is the delta of two other metrics. Signed-off-by: Eric Chlebek <[email protected]>
Signed-off-by: Eric Chlebek <[email protected]>
Signed-off-by: Eric Chlebek <[email protected]>
Signed-off-by: Eric Chlebek <[email protected]>
Signed-off-by: Eric Chlebek <[email protected]>
Signed-off-by: Eric Chlebek <[email protected]>
Signed-off-by: Eric Chlebek <[email protected]>
Signed-off-by: Eric Chlebek <[email protected]>
Signed-off-by: Eric Chlebek <[email protected]>
Signed-off-by: Eric Chlebek <[email protected]>
The cluster_enabled metric does not parse cleanly, so I have dropped it for now. Signed-off-by: Eric Chlebek <[email protected]>
Signed-off-by: Eric Chlebek <[email protected]>
Signed-off-by: Eric Chlebek <[email protected]>
Signed-off-by: Eric Chlebek <[email protected]>
Signed-off-by: Eric Chlebek <[email protected]>
I've hopefully covered all of the feedback given, thanks to all who review. |
Any feedback from code owners would be appreciated, thanks! |
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. |
Any review here would be appreciated, even a rejection. Thanks! |
gauge: | ||
value_type: int |
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.
why not non-monotonic sum? Same Q for redis.stats.tracking_total_keys
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 think that this observed value represents the total amount of memory overhead, so unless I'm mistaken a gauge is the correct representation?
gauge: | ||
value_type: int | ||
|
||
redis.stats.tracking_total_keys: |
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.
Do we need the stats
part? Seems redundant
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 stats part of the name just relates to redis' naming of this key. I think it would be fine to change, I only chose to retain this part of the name just for the sake of easy searching.
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Description
This commit adds metrics to the redis receiver which are available to users that are using the telegraf collector. Providing greater parity here can help users that are attempting to switch from the telegraf collector to the otel collector.
Link to tracking issue
Fixes #39859
Testing
TODO. Seeking assistance on what testing if any needs to be done here, as it's not clear to me if the generated tests are sufficient.
Documentation
Documentation automatically added by mdatagen.