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

[Windows] Add custom conditions support for Perfmon #12830

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mholttech
Copy link
Contributor

@mholttech mholttech commented Feb 18, 2025

Proposed commit message

Enable using conditionals to apply custom perfmon queries to specific systems.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.
  • I have verified that any added dashboard complies with Kibana's Dashboard good practices

Related issues

@mholttech mholttech requested review from a team as code owners February 18, 2025 19:44
@pierrehilbert pierrehilbert added Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team [elastic/elastic-agent-data-plane] Team:Security-Windows Platform Security Windows Platform Team [elastic/sec-windows-platform] labels Feb 19, 2025
@elasticmachine
Copy link

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@elasticmachine
Copy link

Pinging @elastic/sec-windows-platform (Team:Security-Windows Platform)

@belimawr belimawr self-requested a review February 25, 2025 20:59
Copy link
Contributor

@belimawr belimawr left a comment

Choose a reason for hiding this comment

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

@mholttech you missed updating the version on manifest.yml

Comment on lines 2 to 4
{{#if condition }}
condition: ${host.platform} == 'windows' and {{ condition }}
{{ else }}
Copy link
Contributor

Choose a reason for hiding this comment

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

The only problem with this approach is that condtition is a space character ( ), {{#if condition }} will evaluate to true, rendering

condition: ${host.platform} == 'windows' and

Which will make the Elastic-Agent fail to start the input with the following error:

PS C:\elastic-agent-8.17.2-windows-x86_64> & 'C:\Program Files\Elastic\Agent\elastic-agent.exe' status
+- fleet
¦  +- status: (HEALTHY) Connected
+- elastic-agent
   +- status: (FAILED) Invalid component model: rendering inputs failed: invalid condition "${host.platform} == 'windows' and": condition line 1 column 33: mismatched input '<EOF>' expecting {TRUE, FALSE, FLOAT, NUMBER, NOT, NAME, STEXT, DTEXT, '(', '[', '{', '$${', '${'}
PS C:\elastic-agent-8.17.2-windows-x86_64>

The way it shows in Fleet UI is not the most clear either:
2025-02-25_17-53

If I recall correctly I tried something similar in the past, the solution was to have the whole condition in the field, so ${host.platform} == 'windows' would be the default value allowing users to freely modify it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The system integration has a similar situation: a default for condition but we also want to allow the users to add/overwrite it. We ended up using a default and documenting this default, so users always have an easy way to revert to the default behaviour.

Here is the example:

- name: condition
title: Condition
description: |
Condition to filter when to apply this input. Refer to
[Host provider](https://www.elastic.co/guide/en/fleet/current/host-provider.html)
to find the available keys and to
[Conditions](https://www.elastic.co/guide/en/fleet/current/dynamic-input-configuration.html#conditions)
on how to use the available keys in conditions. It defaults to
'${host.os_version} == "12 (bookworm)" or (${host.os_platform} == "amzn" and ${host.os_version} == "2023")'
type: text
multi: false
required: false
show_user: true
default: ${host.os_version} == "12 (bookworm)" or (${host.os_platform} == "amzn" and ${host.os_version} == "2023")

Copy link
Contributor Author

@mholttech mholttech Feb 25, 2025

Choose a reason for hiding this comment

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

That's odd. I'll go ahead and make the change and update the manifest version but it seems like there might be some inconsitencies on how the agent handles this in various situations.

I grabbed this particular pattern from a k8s datastream that i'm familiar with

{{#if condition }}
condition: ${kubernetes.labels.{{~scheduler_label_key~}} } == '{{scheduler_label_value}}' and {{ condition }}
{{ else }}
condition: ${kubernetes.labels.{{~scheduler_label_key~}} } == '{{scheduler_label_value}}'
{{/if}}

Copy link
Contributor

Choose a reason for hiding this comment

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

(...) it seems like there might be some inconsitencies on how the agent handles this in various situations.

Integrations are owned by different teams and we have a huge number of integrations, sometimes different teams have different approaches to solve the same problem.

type: text
multi: false
required: false
show_user: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this hide it from the integration configuration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

show_user: false puts this under "Advanced Options" instead of making it always appear. I figured that would be a good default for this one because it only needs to be modified in specific use cases, especially since it looks like we need to change the implementation slightly per @belimawr's comments

https://github.com/elastic/package-spec/blob/964c4a69e024cc464c4808720ba0db9f001a82a7/spec/integration/data_stream/manifest.spec.yml#L119-L123

Copy link
Contributor

Choose a reason for hiding this comment

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

I like keeping it under the "Advanced options"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team [elastic/elastic-agent-data-plane] Team:Security-Windows Platform Security Windows Platform Team [elastic/sec-windows-platform]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Windows]: Enable custom conditionals for Perfmon
5 participants