Skip to content

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

Merged
ssoroka merged 2 commits intoinfluxdata:masterfrom
yquansah:yq-redis-list
Sep 30, 2020
Merged

feat: add functionality to get values from redis commands#8196
ssoroka merged 2 commits intoinfluxdata:masterfrom
yquansah:yq-redis-list

Conversation

@yquansah
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
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.

Comment on lines +107 to +110
# command = ["get", "sample-key"]
# key = "sample-key-value"
# type = "string"

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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