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 key metricset #9657

Merged
merged 11 commits into from
Dec 21, 2018
Merged

Conversation

jsoriano
Copy link
Member

Add redis key metricset to collect information about keys like length, type or TTL.

Fix #9582

@jsoriano jsoriano added enhancement module review Metricbeat Metricbeat needs_backport PR is waiting to be backported to other branches. Team:Integrations Label for the Integrations team v6.7.0 labels Dec 19, 2018
@jsoriano jsoriano requested a review from a team as a code owner December 19, 2018 02:25
metricbeat/module/redis/redis.go Outdated Show resolved Hide resolved
metricbeat/module/redis/redis.go Show resolved Hide resolved
metricbeat/module/redis/key/key.go Show resolved Hide resolved
@jsoriano jsoriano force-pushed the metricbeat-redis-keys branch from d400021 to a6ce610 Compare December 19, 2018 02:31
@jsoriano jsoriano force-pushed the metricbeat-redis-keys branch from a6ce610 to 39aebc0 Compare December 19, 2018 02:31
@jsoriano
Copy link
Member Author

jenkins, test this

@jsoriano
Copy link
Member Author

jenkins test this please

IdleTimeout time.Duration `config:"idle_timeout"`
Network string `config:"network"`
MaxConn int `config:"maxconn" validate:"min=1"`
Password string `config:"password"`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Config and pool initialization can be probably reused in other metricsets, to be fixed in a follow up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue created for follow ups #9678

// Close connections
func (m *MetricSet) Close() error {
return m.pool.Close()
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be added in other metricsets too, to be added in the follow-up PR.

Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Left a few minor comments.

metricbeat/module/redis/key/_meta/docs.asciidoc Outdated Show resolved Hide resolved
metricbeat/module/redis/key/_meta/docs.asciidoc Outdated Show resolved Hide resolved
metricbeat/module/redis/key/_meta/fields.yml Outdated Show resolved Hide resolved
if err != nil {
return nil, err
}
if keyType == "none" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could make this a constant.

Copy link
Member Author

@jsoriano jsoriano Dec 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Umm, also for the rest of types? Maybe the redis library already has constants, I'll take a look.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using constants for types names now.

return nil, err
}

info := map[string]interface{}{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use common.MapStr here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file was not using MapStr or even common at the moment, I didn't want to add it now, I don't see it will bring anything here.

@@ -29,6 +29,15 @@ import (
rd "github.com/garyburd/redigo/redis"
)

const (
TypeNone = "none"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exported const TypeNone should have comment (or a comment on this block) or be unexported

Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can decide about removing beta later. Failures do not seem to be related.

[[metricbeat-metricset-redis-key]]
=== Redis key metricset

beta[]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we go GA with this directly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's go for GA, I forgot to add the beta warning in the metricset in any case 😄

@jsoriano
Copy link
Member Author

I will add a dashboard for keys in a follow-up PR.

@jsoriano jsoriano merged commit 5dbe803 into elastic:master Dec 21, 2018
@jsoriano jsoriano deleted the metricbeat-redis-keys branch December 21, 2018 09:39
@jsoriano jsoriano removed the needs_backport PR is waiting to be backported to other branches. label Jan 4, 2019
jsoriano added a commit to jsoriano/beats that referenced this pull request Jan 4, 2019
Add redis key metricset to collect information about keys
like length, type or TTL.

(cherry picked from commit 5dbe803)
jsoriano added a commit that referenced this pull request Jan 7, 2019
Add redis key metricset to collect information about keys
like length, type or TTL.

(cherry picked from commit 5dbe803)

Add a dashboard with details for Redis keys using the new key metricset.
Visualizations included show lists length, what is useful when redis is used
for queuing, and average strings sizes and TTLs, what is useful when Redis
is used as a cache.

(cherry picked from commit ac04c32)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants