Skip to content
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

[Receiver/redisreceiver] Support More Redis Metrics #11090

Merged
merged 1 commit into from
Jul 5, 2022

Conversation

singku
Copy link
Contributor

@singku singku commented Jun 16, 2022

Descriptions

  • Add support for maxmemory metric [gauge]
  • Add support for role metric [updown counter]
    With replica and primary added as an attribute
  • Add support for cmdstat metric
    Currently add calls and usec as two new metrics [sum], and cmd as the attribute for both.
  • Fixed info call with all parameter because the comments says all of the 'sections' and command stats is not in the default section.

Related Discussions
open-telemetry/opentelemetry-proto#401
#11025

Tests
Manual tests to see that metrics are added.
Also the existing tests has been updated to fit the new metrics

Documentation
In-place documents added. There are also auto generated document for new metrics.

@singku singku requested a review from a team June 16, 2022 22:40
@singku singku requested a review from dmitryax as a code owner June 16, 2022 22:40
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 17, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: singku / name: Andrew Tang (2968a80510b8e63c55b03e0280a8b09a97d2fb76)

@singku
Copy link
Contributor Author

singku commented Jun 17, 2022

@codeboten @jpkrohling could someone of you approve the workflow to see if this PR pass the checks? Thanks.

@singku
Copy link
Contributor Author

singku commented Jun 21, 2022

Ping for an update.

Copy link
Member

@pirgeo pirgeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at this mainly for the "Enum as Gauge" perspective, as I have not yet spent a lot of time with receivers on the collector. Someone more familiar with the receiver concept might have more insights into this topic.

receiver/redisreceiver/documentation.md Outdated Show resolved Hide resolved
receiver/redisreceiver/documentation.md Outdated Show resolved Hide resolved
receiver/redisreceiver/metadata.yaml Show resolved Hide resolved
@singku
Copy link
Contributor Author

singku commented Jun 22, 2022

@codeboten @mx-psi can we re-assign this to someone? Both the reviewer and assignee are not available till July or August.

Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the changelog.

Another Q: Are all the new metrics that significant to be enabled by default? If we do that, this is going to be a breaking change. I recommend adding them as optional metrics.

receiver/redisreceiver/metadata.yaml Outdated Show resolved Hide resolved
@singku singku force-pushed the newMetrics branch 2 times, most recently from c84143d to 4c0d7f3 Compare July 1, 2022 18:59
@singku
Copy link
Contributor Author

singku commented Jul 1, 2022

@dmitryax I have removed version metric and changed role metric to be an updowncounter. Also added a change log entry.

@dmitryax
Copy link
Member

dmitryax commented Jul 1, 2022

Thanks @singku . I think my last Q is still not answered:

Are all the new metrics that significant to be enabled by default? If we do that, this is going to be a breaking change. I recommend adding them as optional metrics.

@dmitryax
Copy link
Member

dmitryax commented Jul 1, 2022

Also please update the PR description

@singku
Copy link
Contributor Author

singku commented Jul 1, 2022

If they are added as optional. How consumers can config their OTel redisreceiver config to enable them? I didn't find an example.

@dmitryax
Copy link
Member

dmitryax commented Jul 1, 2022

They are added as default metrics. It means that user will start receiving them unexpectedly after the upgrade. Optional metrics are marked as enabled: false in metadata.yaml

@singku
Copy link
Contributor Author

singku commented Jul 1, 2022

I understand that. I mean is there a way to configure at runtime so as to enable these optional metrics? If not, consumer has to pull the code and modify it then rebuild the binary to do so.

Or we could re-enable them by default in next breaking change PR?

@dmitryax
Copy link
Member

dmitryax commented Jul 1, 2022

No, all the metrics can be enabled or disabled by configuring the receiver, e.g.:

receivers:
  redis: 
    metrics:
      <metric_name>:
        enabled: <true|false>

@singku
Copy link
Contributor Author

singku commented Jul 1, 2022

Sounds good! I have made the change.

Note that since maxmeory is by default disabled, for consumer to enable metrics at runtime so recorder is added. This result in recorder count + 6 keyspace metrics count != the generated datapoint counts. Test needs to be adjusted.

Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @pirgeo can you please take a another look since you were involved?

Copy link
Member

@pirgeo pirgeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the changes - LGTM!

@singku
Copy link
Contributor Author

singku commented Jul 5, 2022

@dmitryax @pirgeo @bertysentry thanks for all the comments to this CL. @dmitryax feel free to merge it with appropriate message.

@dmitryax dmitryax merged commit e368e13 into open-telemetry:main Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants