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

Add Redis Sentinel Input Plugin #5978

Open
wants to merge 12 commits into
base: master
from

Conversation

@adamflott
Copy link
Contributor

adamflott commented Jun 11, 2019

Fixed PR #5487

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.
Fixed PR #5487
@adamflott

This comment has been minimized.

Copy link
Contributor Author

adamflott commented Jun 11, 2019

If you'd like me to add more tests for the other measurements I can do that, wanted to get an initial review before I spent more time on it.

@adamflott

This comment has been minimized.

Copy link
Contributor Author

adamflott commented Aug 4, 2019

@danielnelson Any update? We'd like to start pulling my PR changes into mainline

Copy link
Member

glinton left a comment

Thanks

etc/telegraf.conf Outdated Show resolved Hide resolved
plugins/inputs/redis_sentinel/README.md Show resolved Hide resolved
plugins/inputs/redis_sentinel/README.md Outdated Show resolved Hide resolved
plugins/inputs/redis_sentinel/redis_sentinel.go Outdated Show resolved Hide resolved
plugins/inputs/redis_sentinel/redis_sentinel.go Outdated Show resolved Hide resolved
plugins/inputs/redis_sentinel/redis_sentinel.go Outdated Show resolved Hide resolved
plugins/inputs/redis_sentinel/redis_sentinel.go Outdated Show resolved Hide resolved
plugins/inputs/redis_sentinel/redis_sentinel_test.go Outdated Show resolved Hide resolved
plugins/inputs/redis_sentinel/redis_sentinel_test.go Outdated Show resolved Hide resolved
@adamflott

This comment has been minimized.

Copy link
Contributor Author

adamflott commented Aug 15, 2019

@glinton will address tomorrow, thanks for the review!

@joeymiller

This comment has been minimized.

Copy link

joeymiller commented Sep 4, 2019

Any update on merge? Excited to see this work.

@adamflott

This comment has been minimized.

Copy link
Contributor Author

adamflott commented Sep 5, 2019

Any update on merge? Excited to see this work.

Soon. Personal and work life have taken a priority to this.

@adamflott

This comment has been minimized.

Copy link
Contributor Author

adamflott commented Sep 13, 2019

Still trying to figure out why this the tests are failing, the output isn't terribly useful.

	accumulator.go:341: 
			Error Trace:	accumulator.go:341
			            				redis_sentinel_test.go:87
			Error:      	unknown measurement redis_sentinel with tags map[host:redis.net]
			Test:       	TestRedisSentinelParseInfo
@danielnelson

This comment has been minimized.

Copy link
Contributor

danielnelson commented Sep 13, 2019

I recommend using testutil.MustMetric and testutil.RequireMetricsEqual for testing metrics instead of the acc.AssertContainsTaggedFields method. The output is a bit more helpful.

Here is an example of use, you can ignore the metric time using the testutil.IgnoreTime:

https://github.com/influxdata/telegraf/blob/master/plugins/processors/pivot/pivot_test.go

Adam Flott and others added 4 commits Sep 24, 2019
Adam Flott
Use `uptime_ns` per upstream request and get tests passing
Adam Flott
Record all errors with accumulator AddError()
- Add tests against `sentinel {masters,sentinels,replicas}`
- Convert `info all` test to use testutil.RequireMetricsEqual
- Change has-quorum to int (from bool)
@adamflott

This comment has been minimized.

Copy link
Contributor Author

adamflott commented Oct 4, 2019

Everything (code, tests, documentation) should be in now, please re-review. I'd love to get this in for 1.13 time

plugins/inputs/redis_sentinel/redis_sentinel_test.go Outdated Show resolved Hide resolved

- fields:
- config-epoch (int)
- down-after-milliseconds (int)

This comment has been minimized.

Copy link
@glinton

glinton Oct 4, 2019

Member

Would down_after_ns be feasable (convert ms to ns as well)?


- fields:
- config-epoch (int)
- down-after-milliseconds (int)

This comment has been minimized.

Copy link
@glinton

glinton Oct 4, 2019

Member

Would down_after_ns be feasable (convert ms to ns as well)?

plugins/inputs/redis_sentinel/README.md Outdated Show resolved Hide resolved
plugins/inputs/redis_sentinel/redis_sentinel.go Outdated Show resolved Hide resolved
adamflott added 3 commits Oct 4, 2019
Correct README uptime -> uptime_ns
Address a PR suggestion
Per project standards change all metric names to snake case
@adamflott adamflott requested a review from glinton Oct 8, 2019
Ensure compatibility with Go < 1.12
@adamflott

This comment has been minimized.

Copy link
Contributor Author

adamflott commented Oct 14, 2019

Any updates?

The keys coming out did not have their dashes properly replaced with underscores
@glinton
glinton approved these changes Nov 5, 2019
Copy link
Member

glinton left a comment

Sorry for the delays in re-reviewing the requested changes. It looks like you've addressed them all. Thanks.

@adamflott

This comment has been minimized.

Copy link
Contributor Author

adamflott commented Dec 3, 2019

Sorry for the delays in re-reviewing the requested changes. It looks like you've addressed them all. Thanks.

Can I assume this plugin will not be a part of 1.13?

@danielnelson

This comment has been minimized.

Copy link
Contributor

danielnelson commented Dec 3, 2019

Yes, sorry. Let's put it down for 1.14 though for sure.

@danielnelson danielnelson added this to the 1.14.0 milestone Dec 3, 2019
@adamflott

This comment has been minimized.

Copy link
Contributor Author

adamflott commented Jan 25, 2020

@danielnelson ready to merge? Don't wanna miss 1.14 :)

@danielnelson

This comment has been minimized.

Copy link
Contributor

danielnelson commented Jan 28, 2020

The Influxdata team is team building out in Phoenix this week, but I'll try to review on Friday once I'm back home.


const measurementMasters = "redis_sentinel_masters"
const measurementSentinel = "redis_sentinel"
const measurementSentinels = "redis_sentinels"

This comment has been minimized.

Copy link
@danielnelson

danielnelson Feb 3, 2020

Contributor

redis_sentinel_sentinels

const measurementMasters = "redis_sentinel_masters"
const measurementSentinel = "redis_sentinel"
const measurementSentinels = "redis_sentinels"
const measurementReplicas = "redis_replicas"

This comment has been minimized.

Copy link
@danielnelson

danielnelson Feb 3, 2020

Contributor

redis_sentinel_replicas

func (r *RedisSentinel) init(acc telegraf.Accumulator) error {
if r.initialized {
return nil
}
Comment on lines +88 to +91

This comment has been minimized.

Copy link
@danielnelson

danielnelson Feb 3, 2020

Contributor

Rename this function func (r *RedisSentinel) Init() error {, you can remove the initialized boolean.


for i, serv := range r.Servers {
if !strings.HasPrefix(serv, "tcp://") && !strings.HasPrefix(serv, "unix://") {
log.Printf("W! [inputs.redis_sentinel]: server URL found without scheme; please update your configuration file")

This comment has been minimized.

Copy link
@danielnelson

danielnelson Feb 3, 2020

Contributor

Don't add in this support for addresses with a scheme, this is in the normal redis plugin only for backwards compatibility. If the address doesn't begin with tcp or unix return an error instead.


u, err := url.Parse(serv)
if err != nil {
acc.AddError(fmt.Errorf("Unable to parse to address %q: %v", serv, err))

This comment has been minimized.

Copy link
@danielnelson

danielnelson Feb 3, 2020

Contributor

Return an error here, which will cause the failure to be fatal. Since we are removing the warning on line 106, do this first and it will ease checking the scheme.

- fields:
- down_after_milliseconds (int)
- flags (string)
- ip (string)

This comment has been minimized.

Copy link
@danielnelson

danielnelson Feb 3, 2020

Contributor

Does ip differ from sentinel_ip? I think we should skip it as a field.

- link_pending_commands (int)
- link_refcount (int)
- name (string)
- port (int)

This comment has been minimized.

Copy link
@danielnelson

danielnelson Feb 3, 2020

Contributor

Is port the same as sentinel_port? If so let's skip this as well.

- total_connections_received (int)
- total_net_input_bytes (int)
- total_net_output_bytes (int)
- uptime_ns (int, seconds)

This comment has been minimized.

Copy link
@danielnelson

danielnelson Feb 3, 2020

Contributor

List units as nanoseconds.

- replica_port
- source

- fields:

This comment has been minimized.

Copy link
@danielnelson

danielnelson Feb 3, 2020

Contributor

Like with the sentinels, I think we should remove the port, ip, and runid fields.

// ------------------------------------------------------------

// Check other Replicas
replicasCmd := redis.NewSliceCmd("sentinel", "replicas", m["name"])

This comment has been minimized.

Copy link
@danielnelson

danielnelson Feb 3, 2020

Contributor

I found that in versions of Redis < 5, this command is called sentinel slaves <master-group-name>. If we make this call with the older name, the plugin will work with at least back to Redis 3.0, and it continues to work current version.

@sjwang90

This comment has been minimized.

Copy link
Contributor

sjwang90 commented Feb 25, 2020

Hi @adamflott, if you have a chance to take a look at @danielnelson's comments we are seeing if we can get this in to 1.14. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.