Skip to content

Conversation

echlebek
Copy link
Contributor

@echlebek echlebek commented May 5, 2025

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.

@echlebek echlebek requested review from a team and dmitryax as code owners May 5, 2025 20:43
@github-actions github-actions bot added the receiver/redis Redis related issues label May 5, 2025
@github-actions github-actions bot requested a review from hughesjj May 5, 2025 20:44
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]>
@echlebek echlebek force-pushed the redisreceiver-addl-metrics branch from 4adb0ea to 8c7c42e Compare May 5, 2025 20:51
@atoulme
Copy link
Contributor

atoulme commented May 5, 2025

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 atoulme marked this pull request as draft May 5, 2025 20:53
@echlebek
Copy link
Contributor Author

echlebek commented May 5, 2025

@atoulme Will do, thank you.

@atoulme
Copy link
Contributor

atoulme commented May 5, 2025

Moving to draft while you address this feedback. Please mark ready for review once CI passes and the review comments are addressed.

echlebek added 17 commits May 5, 2025 13:54
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]>
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]>
@echlebek
Copy link
Contributor Author

echlebek commented May 9, 2025

I've hopefully covered all of the feedback given, thanks to all who review.

@atoulme
Copy link
Contributor

atoulme commented May 9, 2025

@hughesjj @dmitryax please review as codeowners.

@echlebek
Copy link
Contributor Author

echlebek commented Jun 3, 2025

Any feedback from code owners would be appreciated, thanks!

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jul 16, 2025
@echlebek
Copy link
Contributor Author

Any review here would be appreciated, even a rejection. Thanks!

@github-actions github-actions bot removed the Stale label Jul 17, 2025
Comment on lines +359 to +360
gauge:
value_type: int
Copy link
Member

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

Copy link
Contributor Author

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:
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor

github-actions bot commented Aug 9, 2025

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Aug 9, 2025
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Aug 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

redisreceiver: add metrics that are present in telegraf collector

5 participants