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

import-beats: support multiple streams #380

Merged
merged 18 commits into from
Apr 27, 2020

Conversation

mtojek
Copy link
Contributor

@mtojek mtojek commented Apr 24, 2020

Finally managed to add the missing piece. Got really frustrated...

Changes:

  • convert Golang templates into Handlebars (doesn't support all possible tags, but vast majority of the important one)
  • replace old strings.Replace for template convertion with parsing tree nodes
  • consider multiple streams while compacting
  • add ".hbs" extension to stream agent templates
  • rename existing stream.yml files to stream.yml.hbs

@mtojek mtojek requested a review from ruflin April 24, 2020 14:51
@mtojek mtojek self-assigned this Apr 24, 2020
@@ -155,7 +155,7 @@ func buildPackage(packagesBasePath string, p util.Package) error {
return err
}

err = ioutil.WriteFile(filepath.Join(dirPath, "stream.yml"), []byte(streamFields), 0644)
err = ioutil.WriteFile(filepath.Join(dirPath, "stream.yml.hbs"), []byte(streamFields), 0644)
Copy link
Member

Choose a reason for hiding this comment

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

@andrewkroh You will like this.
@nchaulet @jen-huang Will this break anything at the moment in the config builder? I hope not.

}

func parseStreamConfig(content []byte) (*streamConfigParsed, error) {
mapOfParsed, err := parse.Parse("hello", string(content), "", "", map[string]interface{}{
Copy link
Member

Choose a reason for hiding this comment

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

"hello"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is only single template, so need to define a specific template name hence this "hello world". I can rename it to something meaningful

Copy link
Member

Choose a reason for hiding this comment

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

ah, got it. Something more descriptive would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -0,0 +1,6 @@
type: log
paths:
{{#each paths as |path i|}}
Copy link
Member

Choose a reason for hiding this comment

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

Is the second part as |path i| needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is tricky actually, because the "i" is always present when unrolling the "range". Wouldn't like to detect this in the code. I will let the developer adjust it later on. Is it fine?

Copy link
Member

Choose a reason for hiding this comment

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

SGTM as long as this works on the Kibana side (I assume it does).

// or more contributor license agreements. Licensed under the Elastic License;
// you may not use this file except in compliance with the Elastic License.

var ciscoIOS = (function() {
Copy link
Member

Choose a reason for hiding this comment

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

No need to answer this in this PR but I wonder how this will work. I assume at the moment Filebeat will still ship with this. I assume at the moment this should not be in the package.

@@ -0,0 +1,7 @@
type: log
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be input: log as based on this the mapping to the input is done? Or did this become obsolete because of the path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you're right... I will adjust the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@@ -19,8 +19,8 @@
"/package/multiple-false/0.0.1/manifest.yml",
"/package/multiple-false/0.0.1/docs/README.md",
"/package/multiple-false/0.0.1/dataset/foo/manifest.yml",
"/package/multiple-false/0.0.1/dataset/foo/fields/stream.yml",
"/package/multiple-false/0.0.1/dataset/foo/agent/stream/stream.yml",
"/package/multiple-false/0.0.1/dataset/foo/fields/stream.yml.hbs",
Copy link
Member

Choose a reason for hiding this comment

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

Any chance we could take the .hbs part into a separate PR. We might need here some discussions with Kibana team around this and also makes it easier to reference to it later on if questions come up.

+1 on this change BTW.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... honestly I would keep it here as I already applied in multiple places (simpler option at the moment, preserving we will rename to this). Also it's a parsed template, so definetely a .hbs file. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, in this case we should add a line to the changelog. And perhaps we can do a Github issue instead to describe the modifivation of the change instead and reference this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changelog entry added.

@mtojek mtojek requested a review from ruflin April 27, 2020 11:37
@mtojek mtojek merged commit dadc7ef into elastic:master Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants