Skip to content

Add YAML config source #402

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

Merged
merged 7 commits into from
May 18, 2021
Merged

Conversation

pjanotti
Copy link
Contributor

Adds a config source that can be templatized via https://pkg.go.dev/text/template

Since changing the template typically requires also changing the parameters passed to the template this config source doesn't watch for the template files, i.e., changes to the template file won't trigger reload of the configuration.

@pjanotti pjanotti requested review from a team as code owners May 13, 2021 01:48
@pjanotti
Copy link
Contributor Author

After naming it yaml I'm starting to think that perhaps it will be better to name it template instead. I'm open to other suggestions.

Copy link
Contributor

@jrcamp jrcamp left a comment

Choose a reason for hiding this comment

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

Yeah I think template is a better name.

Comment on lines 34 to 37
component_0: |
$yaml: ./templates/component_template
glob_pattern: /var/**/*.log
format: json
Copy link
Contributor

Choose a reason for hiding this comment

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

Not directly related to this PR but could we do multi-param like:

    $yaml:
       selector: ./templates/component_template
       glob_pattern: /var/**/*.log
       format: json

(Downside of having it within text block is the yaml inside text doesn't get syntax highlighted, etc. when editing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We had some early interactions with it as actual YAML. That ended up being dropped in favor of consistency when used in the single-line format. Actual YAML on single-line didn't look very good and couldn't be concatenated to other values. That said we could have the multiline format being YAML while the single line was our own but that requires a lot more code.

/cc @tigrannajaryan

Copy link
Contributor

Choose a reason for hiding this comment

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

@tigrannajaryan

Would include be a better name?

I like template because all golang template processing is available but no strong opinion in this regard. Let me know if you prefer include.

I think I prefer include since it may be easier to understand for end users the purpose of it. Not strongly opposed to template but it appears a bit obscure, not self-explanatory what it does.

re: stanza plugins

Interesting stuff: at first glance, it seems that they parse the YAML and extract a template from the comments. It should be doable to convert those to the templates supported here.

Yes, I think we can delete the top portion of their YAMLs, which just declare the parameters (I think we can just replace it by comments, I don't think it enables any functionality) and slightly adjust the pipeline portion to match the operators expected by filelog and it should work.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Sorry, replied to a wrong comment thread).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

np, I will rename this config source to include.

@pjanotti
Copy link
Contributor Author

Got CI failures that seem unrelated to the change, closing and re-opening to kick another run.

ci/circleci: deb-package — Your tests failed on CircleCI

ci/circleci: rpm-package — Your tests failed on CircleCI

@pjanotti pjanotti closed this May 18, 2021
@pjanotti pjanotti reopened this May 18, 2021
@pjanotti
Copy link
Contributor Author

close/open doesn't work ...

@pjanotti pjanotti force-pushed the add-yamlconfigsource branch from 83750f5 to 0671c43 Compare May 18, 2021 03:46
@tigrannajaryan
Copy link
Contributor

After naming it yaml I'm starting to think that perhaps it will be better to name it template instead. I'm open to other suggestions.

Would include be a better name?

config_sources:
  include:
receivers:
  filelog: |
    $include: ./plugins/tomcat.yaml
    log_dir: /var/log/tomcat

@tigrannajaryan
Copy link
Contributor

BTW, stanza has a ton of plugins which use the go templating and which I want to reuse for Otelcol logs: https://github.com/observIQ/stanza-plugins/tree/master/plugins

I think we can automate conversion of stanza plugins to filelog-based include/template config sources (or jus tconvert manually, there are not that many).

@pjanotti pjanotti force-pushed the add-yamlconfigsource branch from 0671c43 to 6c832b1 Compare May 18, 2021 14:45
component_0: |
$template: ./templates/component_template
my_glob_pattern: /var/**/*.log
my_format: json
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious if it's possible to use env var expansion here for the template value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is.

@pjanotti
Copy link
Contributor Author

@tigrannajaryan

Would include be a better name?

I like template because all golang template processing is available but no strong opinion in this regard. Let me know if you prefer include.

re: stanza plugins

Interesting stuff: at first glance, it seems that they parse the YAML and extract a template from the comments. It should be doable to convert those to the templates supported here.

@rmfitzpatrick rmfitzpatrick merged commit 280db7f into signalfx:main May 18, 2021
@pjanotti pjanotti deleted the add-yamlconfigsource branch May 18, 2021 19:33
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.

4 participants