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

feat: add functionality to get values from redis commands #8196

Merged
merged 2 commits into from
Sep 30, 2020

Conversation

yquansah
Copy link
Contributor

@yquansah yquansah commented Sep 29, 2020

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

This PR is an attempt to pull additional metrics on redis from arbitrary commands. The logic in this PR accounts for types (string, integer, and float), and we could add more types.

Issue addressed: https://github.com/influxdata/idpe/issues/8711
Community issue Addressed: #1027

@yquansah
Copy link
Contributor Author

Was a little unsure of how exactly to go about editing README

Copy link
Contributor

@ssoroka ssoroka left a comment

Choose a reason for hiding this comment

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

Looks good. The only thing I'd say is that this might be better off as a new plugin. Already you're going to have a hard time changing the metric name if you don't like redis_commands. Also not clear how to control which commands go into which metric, which I guess is not possible currently. That said, I'm not necessarily against merging it into the existing plugin.

plugins/inputs/redis/redis.go Outdated Show resolved Hide resolved
Comment on lines 107 to 110
# command = ["get", "sample-key"]
# key = "sample-key-value"
# type = "string"

Copy link
Contributor

Choose a reason for hiding this comment

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

this looks wrong. Don't you need to specify [[inputs.redis.commands]] ahead of this?

Copy link
Contributor

Choose a reason for hiding this comment

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

consider naming "key" field instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

changes should be copied into readme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is addressed! Thank you for the catch, let me know if there is anything else needed.

@reimda
Copy link
Contributor

reimda commented Sep 29, 2020

Was a little unsure of how exactly to go about editing README

Describe how to use the feature you added. Give examples so people unfamiliar with the feature will be able to understand how to configure it and know what data it will produce.

@yquansah yquansah requested a review from ssoroka September 29, 2020 18:53
@ssoroka ssoroka merged commit 2332e28 into influxdata:master Sep 30, 2020
@yquansah yquansah deleted the yq-redis-list branch November 9, 2020 19:40
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
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.

3 participants