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

[Elastic Agent] Support EQL condition on an input #20784

Closed
blakerouse opened this issue Aug 25, 2020 · 14 comments · Fixed by #20994
Closed

[Elastic Agent] Support EQL condition on an input #20784

blakerouse opened this issue Aug 25, 2020 · 14 comments · Fixed by #20994
Assignees
Labels
Ingest Management:beta2 Group issues for ingest management beta2

Comments

@blakerouse
Copy link
Contributor

Overview

This builds off of #20781 to provide conditions to an input and is the next step in completing support for dynamic inputs #19225. Condition on an input will allow an input to be enabled only if the condition evaluates too true.

Syntax

An input can have a condition defined with the key condition. Without the condition key an input is evaluated to be always be enabled, with the key it is determined based on the result of the condition evaluation.

The syntax will follow a subset of EQL where clause and will not deviate from the design of EQL other than maybe to provide functions that are not yet implemented in EQL. If a function already exists in EQL and we choose to implement it, it will be the same behavior and parameters.

MVP Support

  • and boolean operator
  • or boolean operator
  • == != value comparisons

Examples

Not on Windows

inputs:
  - type: system/load
    condition: "{{host.platform}} != 'windows'"

Only Windows

inputs:
  - type: system/load
    condition: "{{host.platform}} == 'windows'"

Only Windows & AMD64

inputs:
  - type: system/load
    condition: "{{host.platform}} == 'windows' and {{host.architecture}} == 'amd64'"
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ingest-management (Team:Ingest Management)

@ph
Copy link
Contributor

ph commented Aug 25, 2020

@blakerouse Looking a the original proposal for dynamic inputs I am assuming the following is equivalent to your last example?

    condition: 
       - "{{host.platform}} == 'windows'"
       - "{{host.architecture}} == 'amd64'"

Where array items are joined with the and operator?

@blakerouse
Copy link
Contributor Author

After discussion with @ruflin, we agreed that removing the array proposal was better, and to just have condition. The reason, was for the exact reason you asked is it an and or is it or?

Remove the guessing and just make it clearing by reading the YAML using the and boolean operator makes that clear. No need to read documentation to understand if its an and or an or. Its clear by just looking at the YAML that its an and because it will be an and.

@ph
Copy link
Contributor

ph commented Aug 25, 2020

Sound good to me, the boolean operators will make obvious :)

@ruflin
Copy link
Member

ruflin commented Aug 26, 2020

++ on this proposal. If we ever need a list of conditions, we can still introduce it as the name will not conflict.

The proposed syntax for the variable is handelbars. We should probably specify that at first we only support pure variables and nothing else from handlebars for now?

I started to ask myself where exactly in the package spec the conditions will be specified. My current thinking is that it would have to be specified in the manifest of the dataset: https://github.com/elastic/package-storage/blob/production/packages/system/0.5.3/dataset/entropy/manifest.yml An alternative would be to also on the stream level: https://github.com/elastic/package-storage/blob/production/packages/system/0.5.3/dataset/entropy/agent/stream/stream.yml.hbs We probably will need support for it on the Agent on the stream level as the input is system/metrics and then has multiple streams but not all of them run on the same OS.

Assuming it can be specified on the stream level and in the stream template, this might cause some interesting side effects which we have to solve on the Kibana side. Lets assume the following template:

metricsets: ["entropy"]
period: {{period}}
condition: 
  - "{{host.platform}} == 'windows'"

Now Kibana should replace the period variable but leave host.platform in tact as it doesn't have values for it. Trying this in the handlebars playground with only the period variable set, it replaces host.platform with an empty string which it shouldn't. I hope there is a config or workaround to this.

@ruflin
Copy link
Member

ruflin commented Aug 26, 2020

Turns out most of my above comments are covered in #20781, lets continue discussion there.

@blakerouse
Copy link
Contributor Author

@ruflin I think you would need to escape the condition in Kibana.

metricsets: ["entropy"]
period: {{period}}
condition: 
  - "\{{host.platform}} == 'windows'"

Notice the \{{ according to handlebars that would escape it, so once parsed it would become {{ and then Elastic-Agent would replace it.

@ruflin
Copy link
Member

ruflin commented Aug 27, 2020

@blakerouse Would be nice if users would not have to escape conditions as form a package creator perspective it does not really matter where it is evaluated and it would require him to understand this part. I wonder if handlebars supports something like: If you don't know about the var, don't touch it.

@blakerouse
Copy link
Contributor Author

@ruflin I agree that would be nice, but I think name collisions still present a case where you would need to escape it. So it might be better to be explicit and always require escaping instead of having to do it, only in the case of a name collision.

@blakerouse
Copy link
Contributor Author

@ruflin Another option is to make them different. Like Elastic Agent uses {[var]} and the packages use {{var}}.

I know the original goal was to make them the same, but it actually might be less confusing to make them different.

@ruflin
Copy link
Member

ruflin commented Aug 31, 2020

The idea I have in my head, that it could be that some states in the future could even directly be resolved by Kibana. Lets assume for a moment we build a AWS dynamic input and potentially build it in Kibana instead of Agent. Kibana would then already fill in the variables and processors and ship it down to the agent. The person building the package should not care, where the provider magic happens.

For the name collision, my thinking would be that this is not allowed and must be managed by the package dev. He can't have 2 variables with the same names.

@blakerouse
Copy link
Contributor Author

@ruflin I like the idea! I do worry that it might make it harder to understand when writing the package. With Kibana performing the logic, wouldn't the package author know that its going to be rendered by Kibana instead of the Agent?

@ruflin
Copy link
Member

ruflin commented Sep 1, 2020

I'm probably overthinking this but my idea would be that the creator does not care if for example AWS autodiscovery is run by Agent or Kibana. But this is likely too much magic and I'm getting back to your previous proposal to have 2 different syntax? An alternative I stumbled over for the syntax would be ${...:-default} which I assume quite a few users are familiar with?

@blakerouse
Copy link
Contributor Author

Because conditions and variable substitution is split between to issues, I have updated the #19225 with the new variable syntax and the reason for the change.

#20781 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ingest Management:beta2 Group issues for ingest management beta2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants