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

Basic Rollup implementation for Metricbeat #7220

Closed
wants to merge 3 commits into from

Conversation

ruflin
Copy link
Collaborator

@ruflin ruflin commented May 31, 2018

Rollups are especially useful for Metricbeat data. As for most MetricSets we know exactly what fields are ingested we can also specify on how they should be rolled up. For this 2 new config options are introduced in the fields.yml: rollup.term and rollup.metrics. It can be used to specify a field as a term to rollup on and rollup.metrics the metrics which the field should be rolled up to can be specified.

So far an example implementation is done for the system.network metricSet. The generation of the rollup job is currently only available for developers so we start playing around with it. To generate the json doc for a system.network metricset the following command can be run.

go run ../dev-tools/cmd/rollup/rollup.go -metricbeatPath=$GOPATH/src/github.com/elastic/beats/metricbeat -module=system -metricset=network > job.json

Afterwards it can be loaded into Elasticsearch with the following command:

curl -X PUT "localhost:9200/_xpack/rollup/job/metricbeat-system-network" -H 'Content-Type: application/json' -d'{...}'

For loading the jobs directly from Metricbeat there are a few issues:

  • Data has to exist: In case the a job is loaded and the data for it is not in Elasticsearch yet, the loading will fail
  • Delay, interval, cron and page_size are hardcoded: I assume this is something the user wants to specify.

To solve the above it would be interesting to create a job template which does not require the fields to be already there and let the user on job creation time specify the above params.

@ruflin ruflin added in progress Pull request is currently in progress. discuss Issue needs further discussion. libbeat labels May 31, 2018
@@ -91,6 +98,21 @@ func LoadFieldsYaml(path string) (Fields, error) {
return fields, nil
}

func LoadFieldsYamlNoKeys(path string) (Fields, error) {

Choose a reason for hiding this comment

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

exported function LoadFieldsYamlNoKeys should have comment or be unexported

@ruflin
Copy link
Collaborator Author

ruflin commented Aug 16, 2018

One idea to move forward here would be to first only allow users to export rollups for certain metricsets and they have to import it themself. Something like:

metricbeat export rollup system/filesystem

This would print out the json needed for the rollup job. Several advantages here:

  • We can tell the users if he tries to export a rollup job for a metricset that does not have a definition yet
  • We can add additional flags which are required to export like interval and the user has to specify it so we don't have to
  • User can import it as soon as he has data in Elasticsearch.

It's not an out-of-the-box experience but I think it would get us one step closer to at least create rollup jobs.

@polyfractal Any updates from the Rollup side if there are some alternatives?

@polyfractal
Copy link

Afraid nothing new on the Rollup side. We've chatted about template-style implementations, but we're not happy with that solution. Templates are notoriously difficult to validate in Elasticsearch core and can be trappy for users. Rollup would have similar issues (we use the existing data to validate that the rollup config makes sense, e.g. doesn't rollup missing fields, or the wrong data types, etc). We'd like to avoid template-style solutions if possible.

More generally, it probably doesn't make sense for rollups to be enabled by default anyway. A user should want to opt-into the limitations of rollup, rather than being configured out of the box.

I think a cli tool like the above is a good approach, similar to how the beat dashboards are opt-in. So I'm personally ++ to some kind of post-installation tool to help the user, rather than configured by default.

func (p *Processor) Generate() common.MapStr {
return common.MapStr{
"index_pattern": "metricbeat-*",
"rollup_index": "metricbeat_rollup",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@polyfractal Any recommendations on default naming here. I remember that metricbeat-* prefix is probably not the best idea as it matches the same pattern as the default indices.

Choose a reason for hiding this comment

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

metricbeat_rollup seems reasonable. Or maybe rollup_metricbeat or rollup-metricbeat just so it's less likely for someone to confuse the two with an index pattern? I don't think we've really established a convention yet so this is the first :)

You're right that metricbeat-rollup (with the hyphen) would match itself and throw an exception in 6.4.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My favorite would have been metricbeat-rollup as it makes it clear it's almost the same data but I understand that this breaks things.

I'm probably going to stick to rollup-metricbeat-{beat.version} for now as we use - everywhere else for indices.

ruflin added 2 commits August 28, 2018 11:56
Rollups are especially useful for Metricbeat data. As for most MetricSets we know exactly what fields are ingested we can also specify on how they should be rolled up. For this 2 new config options are introduced in the fields.yml: `rollup.term` and `rollup.metrics`. It can be used to specify a field as a `term` to rollup on and `rollup.metrics` the metrics which the field should be rolled up to can be specified.

So far an example implementation is done for the `system.network` metricSet. The generation of the rollup job is currently only available for developers so we start playing around with it. To generate the json doc for a system.network metricset the following command can be run.

```
go run ../dev-tools/cmd/rollup/rollup.go -metricbeatPath=$GOPATH/src/github.com/elastic/beats/metricbeat -module=system -metricset=network > job.json
```

Afterwards it can be loaded into Elasticsearch with the following command:

```
curl -X PUT "localhost:9200/_xpack/rollup/job/metricbeat-system-network" -H 'Content-Type: application/json' -d'{...}'
```

For loading the jobs directly from Metricbeat there are a few issues:

* Data has to exist: In case the a job is loaded and the data for it is not in Elasticsearch yet, the loading will fail
* Delay, interval, cron and page_size are hardcoded: I assume this is something the user wants to specify.

To solve the above it would be interesting to create a job template which does not require the fields to be already there and let the user on job creation time specify the above params.
…-metricset=network --delay=2s` as an example
@@ -277,7 +277,7 @@ func appendFields(fields, appendFields common.Fields) (common.Fields, error) {
return fields, nil
}

func loadYamlByte(data []byte) (common.Fields, error) {
func LoadYamlByte(data []byte) (common.Fields, error) {

Choose a reason for hiding this comment

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

exported function LoadYamlByte should have comment or be unexported

@@ -127,6 +149,44 @@ func (f Fields) HasNode(key string) bool {
return f.hasNode(keys)
}

// HasNode checks if inside fields the given node exists

Choose a reason for hiding this comment

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

comment on exported method Fields.GetNode should be of the form "GetNode ..."

"github.com/elastic/beats/libbeat/template"
)

func GenRollupConfigCmd(name, idxPrefix, beatVersion string) *cobra.Command {

Choose a reason for hiding this comment

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

exported function GenRollupConfigCmd should have comment or be unexported

"github.com/elastic/beats/libbeat/template"
)

func GenRollupConfigCmd(name string) *cobra.Command {

Choose a reason for hiding this comment

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

exported function GenRollupConfigCmd should have comment or be unexported

@ruflin ruflin changed the title [Discuss] Basic Rollup implementation for Metricbeat Basic Rollup implementation for Metricbeat Aug 28, 2018
@ruflin
Copy link
Collaborator Author

ruflin commented Aug 28, 2018

I now added the export rollup command to metricbeat. It can be run for example with:

./metricbeat export rollup --module=system --metricset=network --delay=2s

Currently this only works for the system/network metricset. To which other metricsets should we add it?

genRollupConfigCmd.Flags().String("module", "", "Module to create rollup job for")
genRollupConfigCmd.Flags().String("metricset", "", "Metricset to create rollup job for")
genRollupConfigCmd.Flags().String("index_pattern", "metricbeat-*", "Index pattern to roll up on")
genRollupConfigCmd.Flags().String("rollup_index", "rollup-metricbeat", "Rollup index")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@polyfractal Any recommendation on these defaults? I could make all the fields below required but I would prefer to have some defaults as otherwise it's by default a lot of configuration for the user.

Choose a reason for hiding this comment

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

Some thoughts:

  • I'd use a non-calendar interval (e.g. 60m instead of 1h) since they are a lot more flexible. 1h would limit them to other calendar units which are singular (1h, 1d, 1w, etc). We have a PR up to make this clear in Rollup, as well as converting all the docs over to fixed units
  • If the interval is an hour, there's probably no need to set the cron to every 30s (it'll trigger, see there's nothing to do and go back to sleep). Maybe every 10 or 30 minutes or something similar?
  • delay is debatable. It delays when we process a bucket of time, to allow for out-of-order data to show up. If that's not an issue, I wouldn't even specify a delay. Or set it to something close to the interval, like an hour.
  • I'd go with a bigger page_size, maybe 1000? This is the number of buckets we request from the composite agg, so a small page size == may small searches

@ruflin
Copy link
Collaborator Author

ruflin commented Nov 19, 2018

The problem with the current rollups is that TSVB is not fully supported yet. Work has happened on the Kibana side with a rollup job UI: https://www.elastic.co/guide/en/kibana/6.x/create-and-manage-rollup-job.html This is more useful then loading rollup jobs from the Beats side.

For now I'm going to close this PR as I don't think Beats should load rollup job. The part of this PR that I think will still be valuable in the long term is that fields.yml knows about the potential rollup fields. When Kibana becomes knowledge of the modules / dataset this information can be used to prepoluate the manage rollup UI.

@tsg @monicasarbu FYI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issue needs further discussion. in progress Pull request is currently in progress. libbeat
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants