feat: Add RedisTimeSeries plugin #11054

gkorland · 2022-05-01T08:54:56Z

Required for all PRs:

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

gkorland · 2022-05-01T11:30:43Z

 The formatting of the sampleConfig and the corresponding update in the README.

Done.

 The superfluous length check for metrics in Write().

Done

 Check for address being actually set before constructing the client and report an error if it is not.

Done

 A comment to my question regarding using a mock-package to test this plugin without requiring a real database instance.

I'm not familiar of RedisTimeSeries mock for Go.

 Newly added comment regarding simplification in Write().

Done, also removed the uneeded retry, FT.ADD can create a series if doesn't exist already.

srebhan

Thanks for the nice update @gkorland! I do have only some style suggestions, but the PR looks good overall. There are two more things I want to ask you:

Please sync the README example with the sampleConfig to show all options to the user.
Can you please add a test for the serialization? You can fake a client by introducing an interface with a Do() function and then compare the serialized format to the expected one. This would give us at least some testing in short-mode.

plugins/outputs/redistimeseries/README.md

plugins/outputs/redistimeseries/redistimeseries.go

Co-authored-by: Sven Rebhan <[email protected]>

srebhan

This only leaves us with synchronizing the sampleConfig to the README example...

plugins/outputs/redistimeseries/redistimeseries.go

plugins/outputs/redistimeseries/README.md

srebhan

@gkorland we are almost there. I would comment the optional options (as this is what we do everywhere else). Furthermore, there is a closing quote missing in your TOML section...

plugins/outputs/redistimeseries/README.md

Co-authored-by: Sven Rebhan <[email protected]>

srebhan

Looks good to me. Thanks for keep rocking on this PR @gkorland!

srebhan · 2022-05-25T09:18:27Z

@gkorland it seems like there are some merge conflicts. Can you please rebase this PR on the latest master and resolve the conflicts!?

MyaLongmire

Just one small question and we will be good to go! Thank you for your work on this :)

Can you also run go mod tidy and check back in as that should make the tests pass?

etc/telegraf.conf

plugins/inputs/redis/redis.go

chayim · 2022-06-07T07:31:52Z

Pushed!

srebhan · 2022-06-09T15:27:24Z

@chayim and @gkorland I see the tests failing. This is probably due to an update of the redis libraries. Can you maybe take a look?

thallredis · 2022-06-16T22:15:59Z

@chayim can you look at the failing test?

…eseries-plugin

chayim · 2022-06-28T13:22:21Z

@srebhan I've fixed the merge conflicts, and our code, it passes all of CI, except for the Windows issue in statsd. But, given that this is a timeout.. perhaps it's a red herring? Can you please advise?

srebhan

Can you please fix the error text? Other than this, the PR looks good.

plugins/outputs/redistimeseries/redistimeseries.go

powersj · 2022-06-29T14:52:35Z

plugins/outputs/redistimeseries/redistimeseries_test.go

@@ -0,0 +1,26 @@
+package redistimeseries


As there is zero testing in this PR that would get routinely run, I'd like to see an integration test.

There is an example in the redis input using testcontainers that you can copy and paste from.

@powersj Maybe I misunderstand the testing (and likely)! But doesn't the TestConnectAndWrite in this file, with the MockMetrics cover this use case? I'd like to make sure I'm not missing something - and I'm pretty sure I am.

The point is that you have only a single test and this test isn't run by default (see if testing.Short() { ...Skip...}) as it requires a local instance of redis running. @powersj added some infrastructure to enable integration tests using testcontainers (aka docker images) to verify functionality of plugins.

So the request is to make use of this infrastructure (see the plugin linked above) to at least enable integration tests for your plugin...

Co-authored-by: Sven Rebhan <[email protected]>

srebhan · 2022-07-04T19:37:00Z

@gkorland, @chayim can you please fix the CI issue and address the comment of @powersj?

srebhan · 2022-07-07T14:42:49Z

plugins/outputs/redistimeseries/redistimeseries_test.go

+func TestConnectAndWrite(t *testing.T) {
+	if testing.Short() {
+		t.Skip("Skipping integration test in short mode")
+	}
+
+	address := testutil.GetLocalHost() + ":6379"
+	redis := &RedisTimeSeries{
+		Address: address,
+	}
+
+	// Verify that we can connect to the RedisTimeSeries server
+	err := redis.Connect()
+	require.NoError(t, err)
+
+	// Verify that we can successfully write data to the RedisTimeSeries server
+	err = redis.Write(testutil.MockMetrics())
+	require.NoError(t, err)
+}


To allow integration tests you want something like this

Suggested change

func TestConnectAndWrite(t *testing.T) {

if testing.Short() {

t.Skip("Skipping integration test in short mode")

}

address := testutil.GetLocalHost() + ":6379"

redis := &RedisTimeSeries{

Address: address,

}

// Verify that we can connect to the RedisTimeSeries server

err := redis.Connect()

require.NoError(t, err)

// Verify that we can successfully write data to the RedisTimeSeries server

err = redis.Write(testutil.MockMetrics())

require.NoError(t, err)

}

func TestConnectAndWriteIntegration(t *testing.T) {

if testing.Short() {

t.Skip("Skipping integration test in short mode")

}

servicePort := "6379"

container := testutil.Container{

Image: "redis:alpine",

ExposedPorts: []string{servicePort},

WaitingFor: wait.ForListeningPort(nat.Port(servicePort)),

}

require.NoError(t, container.Start(), "failed to start container")

defer func() {

require.NoError(t, container.Terminate(), "terminating container failed")

}()

redis := &RedisTimeSeries{

Address: fmt.Sprintf("%s:%s", container.Address, container.Ports[servicePort]),

}

// Verify that we can connect to the RedisTimeSeries server

require.NoError(t, redis.Connect())

// Verify that we can successfully write data to the RedisTimeSeries server

require.NoError(t, redis.Write(testutil.MockMetrics()))

}

powersj · 2022-07-18T19:12:58Z

Hi @chayim - @srebhan and I talked and he will write the integration test once this is merged. It is something he wanted to learn anyway.

Can you resolve the test issues and we can land this? Looks like a complaint:

vet: plugins/outputs/redistimeseries/redistimeseries.go:40:10: undeclared name: errors

You should be able to reproduce locally with make check

Thanks!

chayim · 2022-07-19T09:16:04Z

You should be able to reproduce locally with make check

Yes... especially since that was already there, and I just needed to push. Pushing @powersj

telegraf-tiger · 2022-07-19T09:35:36Z

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

🥳 This pull request decreases the Telegraf binary size by -2.02 % for linux amd64 (new size: 143.3 MB, nightly size 146.2 MB)

📦 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_i386.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz
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
		static_linux_amd64.tar.gz

srebhan · 2022-07-20T09:12:37Z

FYI: PR for the integration test can be found in #11529.

@gkorland you might also want to change the link in your documentation to the official Telegraf, not that this PR is in... ;-)

add support for RedisTimeSeries plugin

ba8803c

gkorland mentioned this pull request May 1, 2022

Add support for RedisTimeSeries #5275

Closed

3 tasks

gkorland added 3 commits May 1, 2022 14:10

fix README and example in code

21ff22a

verify redis address is set

27c2e74

add logger

e2317db

gkorland changed the title ~~add support for RedisTimeSeries plugin~~ Add support for RedisTimeSeries plugin May 1, 2022

gkorland added 2 commits May 1, 2022 14:43

simplify write code

9feb1d6

fix lint errors

ed46aba

srebhan reviewed May 9, 2022

View reviewed changes

gkorland and others added 7 commits May 9, 2022 11:47

Update plugins/outputs/redistimeseries/README.md

03c2676

Co-authored-by: Sven Rebhan <[email protected]>

Update plugins/outputs/redistimeseries/redistimeseries.go

1f50c47

Co-authored-by: Sven Rebhan <[email protected]>

Update plugins/outputs/redistimeseries/redistimeseries.go

2503242

Co-authored-by: Sven Rebhan <[email protected]>

Update plugins/outputs/redistimeseries/redistimeseries.go

ff61ece

Co-authored-by: Sven Rebhan <[email protected]>

Update plugins/outputs/redistimeseries/redistimeseries.go

557a6ca

Co-authored-by: Sven Rebhan <[email protected]>

Update plugins/outputs/redistimeseries/redistimeseries.go

3f3aa27

Co-authored-by: Sven Rebhan <[email protected]>

ftm code

61d6c45

srebhan reviewed May 12, 2022

View reviewed changes

plugins/outputs/redistimeseries/redistimeseries.go Show resolved Hide resolved

readme updates

1452de8

srebhan reviewed May 16, 2022

View reviewed changes

plugins/outputs/redistimeseries/README.md Outdated Show resolved Hide resolved

PR comments

cffe9ad

srebhan reviewed May 23, 2022

View reviewed changes

plugins/outputs/redistimeseries/README.md Outdated Show resolved Hide resolved

Update plugins/outputs/redistimeseries/README.md

97de1e2

Co-authored-by: Sven Rebhan <[email protected]>

srebhan approved these changes May 25, 2022

View reviewed changes

srebhan self-assigned this May 25, 2022

srebhan added area/redis new plugin labels May 25, 2022

srebhan changed the title ~~Add support for RedisTimeSeries plugin~~ feat: Add RedisTimeSeries plugin May 25, 2022

Merge branch 'master' into redistimeseries-plugin

eaebdd5

Merge branch 'master' into redistimeseries-plugin

0d5c540

MyaLongmire reviewed May 30, 2022

View reviewed changes

etc/telegraf.conf Show resolved Hide resolved

plugins/inputs/redis/redis.go Outdated Show resolved Hide resolved

go mod tidy

90653df

gkorland and others added 3 commits June 19, 2022 19:53

Merge branch 'master' of github.com:influxdata/telegraf into redistim…

4fde7a0

…eseries-plugin

Merge branch 'master' into redistimeseries-plugin

2f98527

lint fix, mod fix

e12d4d8

sspaink 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 Jun 28, 2022

srebhan reviewed Jun 29, 2022

View reviewed changes

plugins/outputs/redistimeseries/redistimeseries.go Outdated Show resolved Hide resolved

powersj reviewed Jun 29, 2022

View reviewed changes

Update plugins/outputs/redistimeseries/redistimeseries.go

3f57e14

Co-authored-by: Sven Rebhan <[email protected]>

srebhan reviewed Jul 7, 2022

View reviewed changes

srebhan mentioned this pull request Jul 18, 2022

Add integration test for Redis timeseries plugin #11513

Closed

make check fix

ed0f52d

powersj approved these changes Jul 19, 2022

View reviewed changes

powersj merged commit 3c5d71b into influxdata:master Jul 19, 2022

powersj mentioned this pull request Jul 19, 2022

test: add integration test to redis-timeseries #11523

Closed

gkorland deleted the redistimeseries-plugin branch August 17, 2022 05:37

sjwang90 mentioned this pull request Sep 7, 2022

Telegraf 1.24 influxdata/docs-v2#4422

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add RedisTimeSeries plugin #11054

feat: Add RedisTimeSeries plugin #11054

gkorland commented May 1, 2022

gkorland commented May 1, 2022 •

edited

Loading

srebhan left a comment

srebhan left a comment

srebhan left a comment

srebhan left a comment

srebhan commented May 25, 2022

MyaLongmire left a comment •

edited

Loading

chayim commented Jun 7, 2022

srebhan commented Jun 9, 2022

thallredis commented Jun 16, 2022

chayim commented Jun 28, 2022

srebhan left a comment

powersj Jun 29, 2022

chayim Jul 7, 2022

srebhan Jul 7, 2022

srebhan commented Jul 4, 2022

srebhan Jul 7, 2022

powersj commented Jul 18, 2022

chayim commented Jul 19, 2022 •

edited

Loading

telegraf-tiger bot commented Jul 19, 2022

Artifact URLs

srebhan commented Jul 20, 2022

feat: Add RedisTimeSeries plugin #11054

feat: Add RedisTimeSeries plugin #11054

Conversation

gkorland commented May 1, 2022

Required for all PRs:

gkorland commented May 1, 2022 • edited Loading

srebhan left a comment

Choose a reason for hiding this comment

srebhan left a comment

Choose a reason for hiding this comment

srebhan left a comment

Choose a reason for hiding this comment

srebhan left a comment

Choose a reason for hiding this comment

srebhan commented May 25, 2022

MyaLongmire left a comment • edited Loading

Choose a reason for hiding this comment

chayim commented Jun 7, 2022

srebhan commented Jun 9, 2022

thallredis commented Jun 16, 2022

chayim commented Jun 28, 2022

srebhan left a comment

Choose a reason for hiding this comment

powersj Jun 29, 2022

Choose a reason for hiding this comment

chayim Jul 7, 2022

Choose a reason for hiding this comment

srebhan Jul 7, 2022

Choose a reason for hiding this comment

srebhan commented Jul 4, 2022

srebhan Jul 7, 2022

Choose a reason for hiding this comment

powersj commented Jul 18, 2022

chayim commented Jul 19, 2022 • edited Loading

telegraf-tiger bot commented Jul 19, 2022

Artifact URLs

srebhan commented Jul 20, 2022

gkorland commented May 1, 2022 •

edited

Loading

MyaLongmire left a comment •

edited

Loading

chayim commented Jul 19, 2022 •

edited

Loading