feat(inputs.redis): Add node discovery for clusters #15209

jeremycampbell-okta · 2024-04-22T23:12:52Z

Summary

Enables Redis Cluster node discovery for Telegraf Redis plug-in, so all nodes get their Metrics scraped.

Checklist

No AI generated code was used in this PR

Related issues

resolves #15199

telegraf-tiger · 2024-04-22T23:12:58Z

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

srebhan

@jeremycampbell-okta thanks for your contribution! Just a few small comments...

plugins/inputs/redis/redis.go

srebhan · 2024-04-23T13:04:19Z

plugins/inputs/redis/redis.go

+	str, ok := val.(string)
+	if ok {
+		return parseClusterNodes(str)
+	}
+	return nil, fmt.Errorf("could not discover nodes: %w", err)


I would prefer

Suggested change

str, ok := val.(string)

if ok {

return parseClusterNodes(str)

}

return nil, fmt.Errorf("could not discover nodes: %w", err)

str, ok := val.(string)

if !ok {

return nil, fmt.Errorf("could not discover nodes: %w", err)

}

return parseClusterNodes(str)

and maybe fold the parseClusterNodes function into this one...

I liked separating parseClusterNodes to make it more easily/directly testable. Can you say more about why you prefer to fold it in?

(To add to this...I expect maintenance of this function may take some work as/if Redis cluster nodes format changes in the future. As you can see from the tests, there are already 3 variations.)

It's mostly a personal preference but splitting code into two functions, i.e. there is only one caller, where the caller side is very small usually requires the reader to jump between different places and makes following the flow more difficult. Like in this example, the discoverNodes function reads, "do a API call and parse the result". This adds no information to the reader as you cannot see how and what is parsed.

The splitting can be reasonable IMO if a) there are many processing steps, b) a lot of branches or c) complex processing steps. None of those criteria are applicable here as the parsing is fairly "simple" in terms of code...

plugins/inputs/redis/redis.go

telegraf-tiger · 2024-04-23T22:48:45Z

Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	darwin_arm64.tar.gz	windows_arm64.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz	windows_i386.zip
armhf.deb	i386.rpm	freebsd_armv7.tar.gz
i386.deb	ppc64le.rpm	freebsd_i386.tar.gz
mips.deb	riscv64.rpm	linux_amd64.tar.gz
mipsel.deb	s390x.rpm	linux_arm64.tar.gz
ppc64el.deb	x86_64.rpm	linux_armel.tar.gz
riscv64.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_riscv64.tar.gz
		linux_s390x.tar.gz

srebhan

Thanks @jeremycampbell-okta!

powersj

Thanks for jumping on this! ~~Two~~ Three small nits and a question about closing the clients that I'd like your thoughts on. Thanks again!

powersj · 2024-04-24T15:29:31Z

plugins/inputs/redis/README.md

+  ## Enabling this feature triggers the execution of a `cluster nodes` command 
+  ## at each metric gathering interval. This command automatically detects 
+  ## all cluster nodes and retrieves metrics from each of them.
+  # node_discovery = true


The config should show the default values.

Suggested change

# node_discovery = true

# node_discovery = false

powersj · 2024-04-24T15:30:02Z

plugins/inputs/redis/sample.conf

+  ## Enabling this feature triggers the execution of a `cluster nodes` command 
+  ## at each metric gathering interval. This command automatically detects 
+  ## all cluster nodes and retrieves metrics from each of them.
+  # node_discovery = true


Suggested change

# node_discovery = true

# node_discovery = false

powersj · 2024-04-24T15:35:16Z

plugins/inputs/redis/redis.go

+	val, err := client.Do("string", "cluster", "nodes")
+
+	if err != nil {
+		return nil, err
+	}


Suggested change

val, err := client.Do("string", "cluster", "nodes")

if err != nil {

return nil, err

}

val, err := client.Do("string", "cluster", "nodes")

if err != nil {

return nil, err

}

powersj · 2024-04-24T15:43:19Z

plugins/inputs/redis/redis.go

@@ -302,7 +347,7 @@ func (r *Redis) connect() error {
 // Reads stats from all configured servers accumulates stats.
 // Returns one of the errors encountered while gather stats (if any).
 func (r *Redis) Gather(acc telegraf.Accumulator) error {
-	if !r.connected {
+	if !r.connected || r.NodeDiscovery {


Hmm if we re-create the slice of clients during each gather, should we also be calling client.Close() on the existing clients first inside connect()?

I can test this out. I haven't seen any rise in client connectivity in my deployed tests, but this would add some certainty.

telegraf-tiger · 2024-05-14T18:09:43Z

Hello! I am closing this issue due to inactivity. I hope you were able to resolve your problem, if not please try posting this question in our Community Slack or Community Forums or provide additional details in this issue and reqeust that it be re-opened. Thank you!

powersj · 2024-05-15T17:00:19Z

@jeremycampbell-okta I would very much like to see this land. I think the only thing left was closing the connection. Is this something you are still looking into?

jeremycampbell-okta force-pushed the redis-node-discovery branch from a8bbaf7 to 3f4a9f2 Compare April 22, 2024 23:18

feat: Redis plug-in node discovery

ff4bab0

jeremycampbell-okta force-pushed the redis-node-discovery branch from 3f4a9f2 to ff4bab0 Compare April 22, 2024 23:21

jeremycampbell-okta changed the title ~~Redis plug-in node discovery~~ feat: Redis plug-in node discovery Apr 22, 2024

telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Apr 22, 2024

docs: Redis plug-in node discovery

efac4c7

jeremycampbell-okta force-pushed the redis-node-discovery branch from 314a2ff to efac4c7 Compare April 22, 2024 23:31

jeremycampbell-okta added 2 commits April 22, 2024 18:36

docs: Redis plug-in node discovery

75b2d57

delint

479b7e7

jeremycampbell-okta marked this pull request as ready for review April 23, 2024 01:27

Clarify docs

61509bf

srebhan changed the title ~~feat: Redis plug-in node discovery~~ feat(inputs.redis): Add node discovery for clusters Apr 23, 2024

telegraf-tiger bot added area/redis plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Apr 23, 2024

srebhan reviewed Apr 23, 2024

View reviewed changes

srebhan self-assigned this Apr 23, 2024

jeremycampbell-okta added 2 commits April 23, 2024 10:52

Address Review Style Suggestions

2b045c2

delint

0a424c6

srebhan approved these changes Apr 24, 2024

View reviewed changes

srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Apr 24, 2024

srebhan assigned powersj and DStrand1 and unassigned srebhan Apr 24, 2024

powersj reviewed Apr 24, 2024

View reviewed changes

powersj added the waiting for response waiting for response from contributor label Apr 30, 2024

telegraf-tiger bot closed this May 14, 2024

telegraf-tiger bot removed the waiting for response waiting for response from contributor label May 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(inputs.redis): Add node discovery for clusters #15209

feat(inputs.redis): Add node discovery for clusters #15209

jeremycampbell-okta commented Apr 22, 2024

telegraf-tiger bot commented Apr 22, 2024

srebhan left a comment

srebhan Apr 23, 2024

jeremycampbell-okta Apr 23, 2024

jeremycampbell-okta Apr 23, 2024

srebhan Apr 23, 2024

telegraf-tiger bot commented Apr 23, 2024

Artifact URLs

srebhan left a comment

powersj left a comment •

edited

powersj Apr 24, 2024

powersj Apr 24, 2024

powersj Apr 24, 2024

powersj Apr 24, 2024

jeremycampbell-okta Apr 24, 2024

telegraf-tiger bot commented May 14, 2024

powersj commented May 15, 2024

feat(inputs.redis): Add node discovery for clusters #15209

feat(inputs.redis): Add node discovery for clusters #15209

Conversation

jeremycampbell-okta commented Apr 22, 2024

Summary

Checklist

Related issues

telegraf-tiger bot commented Apr 22, 2024

srebhan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

telegraf-tiger bot commented Apr 23, 2024

Artifact URLs

srebhan left a comment

Choose a reason for hiding this comment

powersj left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

telegraf-tiger bot commented May 14, 2024

powersj commented May 15, 2024

powersj left a comment •

edited