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

Filebeat Redis prospector type #4180

Merged
merged 1 commit into from
May 26, 2017
Merged

Conversation

ruflin
Copy link
Member

@ruflin ruflin commented May 3, 2017

This PR adds a new prospector type which reads the slowlog from redis. This slowlog is not in a file but in memory in redis. Because of this filebeat connects to redis and reads out the slowlog. It is important to note that the slow log size is limited in redis that is why after fetching the events from the slowlog filebeat resets the slow log.

Example event looks as following:

{
  "@timestamp": "2017-05-16T06:27:17.000Z",
  "beat": {
    "hostname": "ruflin",
    "name": "ruflin",
    "read_timestamp": "2017-05-16T06:27:19.275Z",
    "version": "6.0.0-alpha2"
  },
  "message": "SET hello world",
  "redis": {
    "slowlog": {
      "args": [
        "world"
      ],
      "cmd": "SET",
      "duration": {
        "us": 11
      },
      "id": 38,
      "key": "hello"
    }
  }
}

All args are combined in the "message" field output for easy retrieval.

@ruflin ruflin added discuss Issue needs further discussion. Filebeat Filebeat in progress Pull request is currently in progress. labels May 3, 2017
Password: "",
}

type Config struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[golint] reported by reviewdog 🐶
exported type Config should have comment or be unexported

@@ -0,0 +1,14 @@
// Redis package contains prospector and harvester to read the redis slow log
Copy link
Collaborator

Choose a reason for hiding this comment

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

[golint] reported by reviewdog 🐶
package comment should be of the form "Package redis ..."

rd "github.com/garyburd/redigo/redis"
)

type Harvester struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[golint] reported by reviewdog 🐶
exported type Harvester should have comment or be unexported

args []string
}

func (h *Harvester) Start(conn rd.Conn) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[golint] reported by reviewdog 🐶
exported method Harvester.Start should have comment or be unexported

rd "github.com/garyburd/redigo/redis"
)

// Stdin is a prospector for stdin
Copy link
Collaborator

Choose a reason for hiding this comment

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

[golint] reported by reviewdog 🐶
comment on exported type Prospector should be of the form "Prospector ..." (with optional leading article)

config Config
}

// NewStdin creates a new stdin prospector
Copy link
Collaborator

Choose a reason for hiding this comment

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

[golint] reported by reviewdog 🐶
comment on exported function NewProspector should be of the form "NewProspector ..."

@@ -215,6 +215,14 @@ filebeat.prospectors:
# Configuration to use stdin input
#- input_type: stdin

#----------------------------- Redis prospector -------------------------------
#- input_type: redis
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest somehow making it more clear that this is strictly about slow logs. People might assume that it's a general Redis input.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is a tricky one. Thinking more about the prospectors I get the impression we will have prospector with multiple harvester types potentially (similar to what we have in metricbeat). For example in file there could be file.csv or file.xml. What if we not only have slowlog for redis? But TBH I'm not sure yet what we should do here. This could also well be overengineered and we will not need it.

For the moment I think I fix the above with call it Redis slowlog prospector in the docs but still redis as the prospector type.

//
// CONFIG SET slowlog-log-slower-than 100
//
// This sets the value of the slow log to 100 micro seconds. All queries taking longer will be reported.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd use 2s or 5s as an example. 100 micros might affect the Redis performance, so we should be careful.

Copy link
Member Author

Choose a reason for hiding this comment

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

changed

//
// This sets the value of the slow log to 100 micro seconds. All queries taking longer will be reported.
//
// As the slow log is in memory, in can be configured how many items it consists:
Copy link
Contributor

Choose a reason for hiding this comment

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

small typo: "it can be configured"

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed


data := util.NewData()
data.Event = common.MapStr{
// TODO: Should we still send the "fetching" timestamp or overwrite it by timestamp?
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, good question. In the end it needs to be the event timestamp, but I see two options:

  • live it like that here, but replace it in the FB redis module
  • already set it hear and also set beat.read_timestamp

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 decided for option 2 for the moment. The reasons is that I think it most cases that is what people expect in @timestamp here. So also without postprocessing in ingest, the data is already correct.

if argsLen >= 2 {
log.key = args[1]
}
// TODO: Could args contain confidential data? Should it be optional?
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess people can use drop_fields if they don't want it.

@ruflin ruflin mentioned this pull request May 10, 2017
14 tasks
@ruflin ruflin force-pushed the fb-redis-approach branch 2 times, most recently from 456e2e0 to 9ba6162 Compare May 15, 2017 11:25
Processors processors.PluginConfig `config:"processors"`
}

// updateState updates the prospector state and forwards the event to the spooler
Copy link
Collaborator

Choose a reason for hiding this comment

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

[golint] reported by reviewdog 🐶
comment on exported method Forwarder.Forward should be of the form "Forward ..."

"github.com/satori/go.uuid"
)

type Harvester struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[golint] reported by reviewdog 🐶
exported type Harvester should have comment or be unexported

args []string
}

func NewHarvester(outlet channel.Outleter, conn rd.Conn) *Harvester {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[golint] reported by reviewdog 🐶
exported function NewHarvester should have comment or be unexported

}
}

func (h *Harvester) Start() error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[golint] reported by reviewdog 🐶
exported method Harvester.Start should have comment or be unexported

return nil
}

func (h *Harvester) Stop() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[golint] reported by reviewdog 🐶
exported method Harvester.Stop should have comment or be unexported


}

func (h *Harvester) ID() uuid.UUID {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[golint] reported by reviewdog 🐶
exported method Harvester.ID should have comment or be unexported

}
}

func (p *Prospector) Stop() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[golint] reported by reviewdog 🐶
exported method Prospector.Stop should have comment or be unexported


}

func (p *Prospector) Wait() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[golint] reported by reviewdog 🐶
exported method Prospector.Wait should have comment or be unexported

@ruflin ruflin force-pushed the fb-redis-approach branch 5 times, most recently from 4a55634 to 36be381 Compare May 16, 2017 06:33
@ruflin ruflin changed the title [WIP] Filebeat Redis prospector type Filebeat Redis prospector type May 16, 2017
@ruflin ruflin added review and removed discuss Issue needs further discussion. labels May 16, 2017
@ruflin
Copy link
Member Author

ruflin commented May 16, 2017

@tsg Ready for an other look. I think we are at the point now where we have to decide on what to do with it :-)

)

// ValidType is a list of all valid input types
// List of valid input types
Copy link
Collaborator

Choose a reason for hiding this comment

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

[golint] reported by reviewdog 🐶
comment on exported var ValidType should be of the form "ValidType ..."

}
}

// Start starts a new redis harvester
Copy link
Collaborator

Choose a reason for hiding this comment

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

[golint] reported by reviewdog 🐶
comment on exported method Harvester.Run should be of the form "Run ..."

@ruflin ruflin force-pushed the fb-redis-approach branch 2 times, most recently from e021c74 to 8a8caba Compare May 19, 2017 14:26
@ruflin
Copy link
Member Author

ruflin commented May 19, 2017

@dedemorton So far I only added the minimial options of docs for this = almost nothing. The reason is that this kind of challenges our existing doc structure we have for prospectors. So far the 2 prospectors shared 95% of the config options. With this change, they can be very different. I'm thinking if we should start to organise docs similar to what we do with the modules and have a block for each prospector type. I will need your help here and suggest we will only tackle this after merging the PR.

@dedemorton
Copy link
Contributor

@ruflin I remember you mentioned this problem awhile back ago in an email, but I haven't had a chance to follow up with you. What release are we targeting for this PR?

@ruflin
Copy link
Member Author

ruflin commented May 22, 2017

@dedemorton Not sure if I mentioned that before (but could be). It's not urgent. If it happens, somewhere around 6.0

@@ -379,6 +379,27 @@ filebeat.prospectors:
# Configuration to use stdin input
#- type: stdin

#------------------------- Redis slowlog prospector ---------------------------
# Experimental: Config options ofr the redis slow log prospector
Copy link
Contributor

Choose a reason for hiding this comment

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

small typo on "ofr".

This PR adds a new prospector type which reads the slowlog from redis. This slowlog is not in a file but in memory in redis. Because of this filebeat connects to redis and reads out the slowlog. It is important to note that the slow log size is limited in redis that is why after fetching the events from the slowlog filebeat resets the slow log.

Example event looks as following:

```
{
  "@timestamp": "2017-05-16T06:27:17.000Z",
  "beat": {
    "hostname": "ruflin",
    "name": "ruflin",
    "read_timestamp": "2017-05-16T06:27:19.275Z",
    "version": "6.0.0-alpha2"
  },
  "message": "SET hello world",
  "redis": {
    "slowlog": {
      "args": [
        "world"
      ],
      "cmd": "SET",
      "duration": {
        "us": 11
      },
      "id": 38,
      "key": "hello"
    }
  }
}
```

All args are combined in the "message" field output for easy retrieval.
@ruflin
Copy link
Member Author

ruflin commented May 26, 2017

@tsg New version pushed
@dedemorton I created #4398 as a follow up Github issue for the docs.

@ruflin ruflin removed the in progress Pull request is currently in progress. label May 26, 2017
@tsg tsg merged commit 4ab7848 into elastic:master May 26, 2017
@ruflin ruflin deleted the fb-redis-approach branch May 26, 2017 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants