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 support for template strings with the remap language #3836

Closed
4 tasks
binarylogic opened this issue Sep 12, 2020 · 3 comments
Closed
4 tasks

Add support for template strings with the remap language #3836

binarylogic opened this issue Sep 12, 2020 · 3 comments
Labels
domain: templating Anything related to templating Vector's configuration values have: should We should have this feature, but is not required. It is medium priority. needs: rfc Needs an RFC before work can begin. type: enhancement A value-adding code change that enhances its existing functionality.

Comments

@binarylogic
Copy link
Contributor

binarylogic commented Sep 12, 2020

I'd like to unify our templating syntax with our remap language. This was inspired by Rune's template string support. The benefits are:

  1. One familiar syntax and function reference for Vector.
  2. Access to all of remap's functions for templating.
  3. Less code to manage.

Option questions

  1. Should we still support control flow (if statements) of some sort? I could see the case for ternary like operators:

    [sinks.s3]
    type = "aws_s3"
    path = "{{ severity >= 3 ? "errors" : "logs"}}"
  2. Are we better off breaking this into smaller issues? For example, can we start with the template string code and unit tests, and then follow it up by implementing it into each relevent sink?

Requirements

  • Depcreate the current template syntax but do not remove it.
  • Preserve backward compatibility with the current syntax. We should be able to detect this easily.
  • Update documentation to reflect the new syntax.
  • Only select functions and if-statements should be supported. Anything that mutates the event, like del, should not.

If we find there are many open questions, let's start with an RFC.

@binarylogic binarylogic added type: enhancement A value-adding code change that enhances its existing functionality. domain: templating Anything related to templating Vector's configuration values labels Sep 12, 2020
@binarylogic binarylogic added this to the 2020-09-14 - The Grid milestone Sep 17, 2020
@binarylogic binarylogic added the have: should We should have this feature, but is not required. It is medium priority. label Sep 17, 2020
@StephenWakely
Copy link
Contributor

There are two issues with this change that need to be clarified.

  1. It isn't that easy to detect if the old syntax or the Remap syntax is being used:
  • A simple regex scan for {{...}} isn't sufficient to detect the old syntax, as there is valid Remap code that could contain this. (eg. contains(.foo, "{{thing}}").
  • We could parse the string with Remap first. If it fails to parse then we could assume it is old syntax. However, this means that we won't be able to capture and report any errors if the user intended to write a Remap template, but made an error. So del(.foo) is not valid Remap in this instance, because del is a mutable function and not allowed. Remap will not parse this, the template will then just assume this is just a constant string to use. No error will be reported.

I think the user will need to explicitly state which syntax they are using in the configuration.

  1. The Loki sink wants a list of the fields that were used in the template. It uses this list to remove these fields from the event sent to Loki. With Remap this list becomes dynamic. So the script if .foo { .bar } else { .baz } means sometimes .bar is used and other times .baz is used.
    We need to decide how this change would be able to accomodate this. Should the parser return all fields used in the script and the Loki sink removes them all? Should it return the fields used directly each time it is run? Or can we remove this feature - it is a regression, but perhaps it isn't that important and the user can fall back to the old syntax if they need it?

I think this needs an RFC.

@binarylogic
Copy link
Contributor Author

I think this needs an RFC.

Go for it. Would you mind opening a separate issue for the RFC work specifically?

@binarylogic
Copy link
Contributor Author

Closing, please see #4905 (comment) for reasoning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: templating Anything related to templating Vector's configuration values have: should We should have this feature, but is not required. It is medium priority. needs: rfc Needs an RFC before work can begin. type: enhancement A value-adding code change that enhances its existing functionality.
Projects
None yet
Development

No branches or pull requests

4 participants