Allow URL format in Metricbeat Redis module #12408

kaiyan-sheng · 2019-06-04T03:25:20Z

Continues with #10920

Original commit message:

Potential fix for #10917

This change will keep the usual functionality when using this approach:

- module: redis
  period: 10s
  # Redis hosts
  hosts: ["127.0.0.1:6379", "127.0.0.2:6379"]

Additionally allowing the use of Redis URL syntax:

- module: redis
  period: 10s
  # Redis hosts
  hosts: ["127.0.0.1:6379", "redis://:testpassword@127.0.0.2:6379"]

The password field will be used for all host:port values. On URL values, the password set in the scheme will have priority.

Co-authored-by: Matias Insaurralde matias@insaurral.de

exekias · 2019-06-04T07:54:51Z

Tests seem to be failing.

Is this change backwards compatible? I think so but want to confirm

kaiyan-sheng · 2019-06-04T20:39:35Z

Tests seem to be failing.

Is this change backwards compatible? I think so but want to confirm

Thanks for reviewing. The failing test is fixed and also added a test to prove it is backwards compatible.

exekias · 2019-06-05T09:02:33Z

LGTM, this will need a changelog

jsoriano · 2019-06-05T15:47:51Z

It seems that the password in redis URIs can be defined as parameter too (redis://HOST[:PORT][?db=DATABASE[&password=PASSWORD]]), should we just replace the dialer function to use redis.DialURI so we don't have to care about all valid URIs?

kaiyan-sheng · 2019-06-05T22:19:01Z

It seems that the password in redis URIs can be defined as parameter too (redis://HOST[:PORT][?db=DATABASE[&password=PASSWORD]]), should we just replace the dialer function to use redis.DialURI so we don't have to care about all valid URIs?

Good point, thank you @jsoriano ! I just changed the CreatePool method to call rd.DialURL directly. But when I was testing this, redis://:password@127.0.0.1:6379 works.

But the parameter way redis://127.0.0.1:6379?password=test does not work. I got an error 2019-06-05T15:37:53.885-0600 ERROR redis/redis.go:70 Error retrieving INFO stats: NOAUTH Authentication required.

When I look for documentation on URL format, I see this: https://www.npmjs.com/package/redis-url#url-format hmmm is the parameter way officially supported or I'm doing something wrong here? 😬

kaiyan-sheng · 2019-06-11T22:52:21Z

@jsoriano I added some 'nasty' parsing in order to make redis://HOST[:PORT][?password=PASSWORD[&db=DATABASE]] format URL working. Turned out redis.DialURL does not recognize the PASSWORD and DATABASE as a part of the query parameters. I had to manually parse PASSWORD and DATABASE out and pass them as DialOptions into DialURL function.

andrewkroh

Just a quick drive-by review. 😄 (I may have commented on things that were already discussed, so sorry if I did.)

metricbeat/module/redis/_meta/docs.asciidoc

metricbeat/module/redis/info/info.go

metricbeat/module/redis/metricset.go

jsoriano

Turned out redis.DialURL does not recognize the PASSWORD and DATABASE as a part of the query parameters. I had to manually parse PASSWORD and DATABASE out and pass them as DialOptions into DialURL function.

Ouch 🙁 docs misguided me, thanks for investigating this! And great you found a solution. Maybe we can contribute back the support for query parameters to the client library we use? 🙂

Is there any reason to don't use URL.Query() or url.ParseQuery() to parse the query arguments?

metricbeat/module/redis/metricset.go

metricbeat/module/redis/redis.go

metricbeat/module/redis/info/info_test.go

jsoriano

Sorry, a few more small things... mainly about handing of database configuration, and some details in error messages and tests.

metricbeat/module/redis/metricset_test.go

metricbeat/module/redis/redis.go

metricbeat/module/redis/metricset.go

jsoriano · 2019-06-13T15:56:19Z

metricbeat/module/redis/metricset.go

 	err := base.Module().UnpackConfig(&config)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to read configuration")
 	}

+	password, database, err := getPasswordDatabase(base.HostData().URI, base.HostData().Password)


Database can be also in the path of the url (redis://[:password@]host[:port][/db-number][?option=value]). For an URL like redis://host/16, will it use the 0 returned here instead of the 16? I think we should add the DialDatabase option only if the database is set as an option.
If it complicates things a lot I am also ok with giving some working examples in the documentation.
In any case it wouldn't be a regression because I think that we cannot configure the database now.

Missed it!! Thanks!!
I added a part in the same function to check db-number from URI itself. Please let me know what you think 👍

Looks good, thanks! We should contribute these things to redigo so all this is done by DialURL 😄

jsoriano

Thanks for all the work here to cover all the cases! 👍

jsoriano · 2019-06-14T08:03:19Z

metricbeat/module/redis/metricset_test.go

+		},
+		{
+			"test redis uri with db number in URI and query field",
+			mb.HostData{URI: "redis://:password1@127.0.0.2:6379/1?password=password2&db=2", Password: "password1"},


kaiyan-sheng · 2019-06-14T12:23:48Z

Thanks for all the reviews!!

kaiyan-sheng requested review from a team as code owners June 4, 2019 03:25

kaiyan-sheng mentioned this pull request Jun 4, 2019

Allow URL format in Metricbeat Redis module #10920

Closed

kaiyan-sheng self-assigned this Jun 4, 2019

exekias closed this Jun 4, 2019

exekias reopened this Jun 4, 2019

kaiyan-sheng added Team:Integrations Label for the Integrations team [zube]: In Review review labels Jun 4, 2019

zube bot added [zube]: Inbox [zube]: In Review and removed [zube]: In Review [zube]: Inbox labels Jun 4, 2019

exekias approved these changes Jun 5, 2019

View reviewed changes

kaiyan-sheng added [zube]: In Review and removed [zube]: Inbox labels Jun 5, 2019

andrewkroh reviewed Jun 11, 2019

View reviewed changes

kaiyan-sheng added in progress Pull request is currently in progress. and removed [zube]: In Review review labels Jun 12, 2019

jsoriano reviewed Jun 12, 2019

View reviewed changes

metricbeat/module/redis/metricset.go Outdated Show resolved Hide resolved

metricbeat/module/redis/redis.go Outdated Show resolved Hide resolved

metricbeat/module/redis/info/info_test.go Show resolved Hide resolved

jsoriano reviewed Jun 13, 2019

View reviewed changes

matiasinsaurralde and others added 11 commits June 13, 2019 13:31

Use custom HostParser when parsing Redis module hosts parameter

431f9f1

rebase and fit formatting

0608023

Add test for old url to test backwards compatible

9b21bc1

Add changelog

db9578a

Use rd.DialURL in CreatePool function

e524a86

Add parser to get password and database from url

6ff622f

Add test for getPasswordDatabase

943cc6a

Add check for db-number from URI

c6d86be

Add password and network back into config

2d2ab54

Remove changes in config.yml

02286a9

Remove changes in config.yml

81db63e

kaiyan-sheng added review and removed in progress Pull request is currently in progress. labels Jun 14, 2019

jsoriano approved these changes Jun 14, 2019

View reviewed changes

kaiyan-sheng merged commit f0a8827 into elastic:master Jun 14, 2019

kaiyan-sheng deleted the redis-multiple-servers branch June 14, 2019 12:25

kaiyan-sheng mentioned this pull request Jun 14, 2019

Multiple Redis Servers with Passwords #10917

Closed

jsoriano mentioned this pull request Jul 11, 2019

Redis key metricset doesn't use db number if set in the host url #12874

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow URL format in Metricbeat Redis module #12408

Allow URL format in Metricbeat Redis module #12408

kaiyan-sheng commented Jun 4, 2019

exekias commented Jun 4, 2019

kaiyan-sheng commented Jun 4, 2019

exekias commented Jun 5, 2019

jsoriano commented Jun 5, 2019 •

edited

Loading

kaiyan-sheng commented Jun 5, 2019 •

edited

Loading

kaiyan-sheng commented Jun 11, 2019

andrewkroh left a comment •

edited

Loading

jsoriano left a comment

jsoriano left a comment

jsoriano Jun 13, 2019

kaiyan-sheng Jun 13, 2019

jsoriano Jun 13, 2019

jsoriano left a comment

jsoriano Jun 14, 2019

kaiyan-sheng commented Jun 14, 2019

Allow URL format in Metricbeat Redis module #12408

Allow URL format in Metricbeat Redis module #12408

Conversation

kaiyan-sheng commented Jun 4, 2019

exekias commented Jun 4, 2019

kaiyan-sheng commented Jun 4, 2019

exekias commented Jun 5, 2019

jsoriano commented Jun 5, 2019 • edited Loading

kaiyan-sheng commented Jun 5, 2019 • edited Loading

kaiyan-sheng commented Jun 11, 2019

andrewkroh left a comment • edited Loading

Choose a reason for hiding this comment

jsoriano left a comment

Choose a reason for hiding this comment

jsoriano left a comment

Choose a reason for hiding this comment

jsoriano Jun 13, 2019

Choose a reason for hiding this comment

kaiyan-sheng Jun 13, 2019

Choose a reason for hiding this comment

jsoriano Jun 13, 2019

Choose a reason for hiding this comment

jsoriano left a comment

Choose a reason for hiding this comment

jsoriano Jun 14, 2019

Choose a reason for hiding this comment

kaiyan-sheng commented Jun 14, 2019

jsoriano commented Jun 5, 2019 •

edited

Loading

kaiyan-sheng commented Jun 5, 2019 •

edited

Loading

andrewkroh left a comment •

edited

Loading