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 the option to generate the template into a file #4323

Merged
merged 4 commits into from
May 18, 2017

Conversation

tsg
Copy link
Contributor

@tsg tsg commented May 16, 2017

Part of #3654.

This adds two settings: setup.template.output_to_file.path and setup.template.output_to_file.version. The version refers to the ES version and is optional, we'll use the Beats version if not specified. I put it under output_to_file to make it clear that it only applies when outputting to a file:

To generate a config, one can do:

./metricbeat -e -E setup.template.output_to_file.path=template.json

In the current version, the Beat automatically stops after generating the template, but the output might be slightly confusing:

2017/05/16 09:55:02.043671 load.go:116: INFO Template for Elasticsearch 6.0.0-alpha2 written to: template.json
2017/05/16 09:55:02.043717 beat.go:538: CRIT Exiting: Stopping after successfully writing the template to the file.
Exiting: Stopping after successfully writing the template to the file.

IMO this is better than the alternative of leaving it running.

To generate the template for the 2.x version, one can do:

./metricbeat -e -E setup.template.output_to_file.path=template.json -E setup.template.output_to_file.version=2.4.0

Remaining TODOs:

  • System test
  • Docs
  • Changelog

@tsg tsg added :Templates review in progress Pull request is currently in progress. libbeat labels May 16, 2017
@ruflin
Copy link
Member

ruflin commented May 16, 2017

Perhaps we should discuss again with after --setup the beat should stop? Because then one could do:

./metricbeat --setup -e -E setup.template.output_to_file.path=template.json

Or assuming -S is implies --setup and prefixes -E with the setup namespace. Then the following could be used:

./metricbeat -S template.output_to_file.path=template.json


// XXX: Should we kill the Beat here or just continue?
return fmt.Errorf("Stopping after successfully writing the template to the file.")
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 🐶
error strings should not be capitalized or end with punctuation or a newline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, dear reviewdog, we know that the error is final, so the punctuation looks better to the user.

@@ -80,6 +82,41 @@ func (l *Loader) Load() error {
return nil
}

func (l *Loader) Generate() 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 Loader.Generate should have comment or be unexported

@tsg
Copy link
Contributor Author

tsg commented May 16, 2017

@ruflin Yeah, I guess we should review the whole -setup mechanism, maybe we can unify the handling in such a way that it makes sense. The beats-tools is tempting, but we'll need to build it for all supported platforms, which makes me a bit worried about the packaging overhead (especially since it's not actually a beat).

I have the feeling we can't get a nice solution as part of this PR, so the question is what do we want for the time being? IMO stopping is much nicer in this case, but it's inconsistent with -setup, that's true :(. I'm still leaning towards stopping.

@ruflin
Copy link
Member

ruflin commented May 16, 2017

Agree that we can't find a good solution as part of this PR. +1 on stopping as if you want to manually load the template, you don't want to already send data.

@tsg tsg removed the in progress Pull request is currently in progress. label May 17, 2017
Copy link
Member

@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.

I left a few comments which we should handle in a follow up. Will merge it for now as this is a big step forward.

@@ -469,8 +469,30 @@ func (b *Beat) registerTemplateLoading() error {
}
}

esConfig := b.Config.Output["elasticsearch"]
// Check if outputting to file is enabled, and output to file if it is
Copy link
Member

Choose a reason for hiding this comment

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

This comment confused me at first because the check for the file only happens later, but I see you meant it for the "complete" block

Settings templateSettings `config:"settings"`
OutputToFile OutputToFile `config:"output_to_file"`
Copy link
Member

Choose a reason for hiding this comment

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

Not too much of a fan of the config name, but well :-)

@@ -80,6 +82,43 @@ func (l *Loader) Load() error {
return nil
}

// Generate generates the template and writes it to a file based on the configuration
// from `output_to_file`.
func (l *Loader) Generate() error {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I didn't realize we have that utility already.

@@ -9,4 +9,11 @@ def setUpClass(self):
self.beat_name = "mockbeat"
self.beat_path = os.path.abspath(os.path.join(os.path.dirname(__file__), "../../"))
self.test_binary = self.beat_path + "/libbeat.test"
self.beats = [
"filebeat",
Copy link
Member

Choose a reason for hiding this comment

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

Looks like a beats specifics dependency in libbeat.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, only needed for those skipped tets for now, I can delete them.

beats = ["metricbeat", "packetbeat", "filebeat", "winlogbeat"]

for beat in beats:
for beat in self.beats:
Copy link
Member

Choose a reason for hiding this comment

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

be aware that these tests are currently skipped.

@ruflin ruflin merged commit f9c0af6 into elastic:master May 18, 2017
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