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

Agentbeat tests based on agentbeat.spec.yml #41074

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

oakrizan
Copy link
Contributor

@oakrizan oakrizan commented Oct 2, 2024

Proposed commit message

Added checks for Agentbeat that execute all unique commands for inputs from agentbeat.spec.yml and validates that those can start without failures.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

Related issues

Logs

Buildkite builds: https://buildkite.com/elastic/beats-xpack-agentbeat/builds?branch=oakrizan%3Aagentbeat-integration-tests-spec

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Oct 2, 2024
Copy link
Contributor

mergify bot commented Oct 2, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @oakrizan? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit

Copy link
Contributor

mergify bot commented Oct 2, 2024

backport-8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

@mergify mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Oct 2, 2024
@oakrizan oakrizan force-pushed the agentbeat-integration-tests-spec branch 25 times, most recently from 53931f7 to ffafe79 Compare October 2, 2024 14:24
@oakrizan oakrizan force-pushed the agentbeat-integration-tests-spec branch from b496aa8 to 2bae33b Compare October 18, 2024 09:31
@oakrizan oakrizan force-pushed the agentbeat-integration-tests-spec branch from 2bae33b to 4740fbe Compare October 18, 2024 11:10
@oakrizan oakrizan marked this pull request as ready for review October 18, 2024 11:55
@oakrizan oakrizan requested review from a team as code owners October 18, 2024 11:55
@pierrehilbert pierrehilbert requested review from blakerouse and rdner and removed request for khushijain21 October 18, 2024 12:10
@pierrehilbert pierrehilbert added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Oct 18, 2024
@elasticmachine
Copy link
Collaborator

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

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Oct 18, 2024
@oakrizan oakrizan added backport-8.15 Automated backport to the 8.15 branch with mergify backport-8.16 Automated backport with mergify ci Team:Ingest-EngProd labels Oct 18, 2024
@cmacknz
Copy link
Member

cmacknz commented Oct 21, 2024

This is the first part of a solution, the spec parsing works, but we aren't actually testing enough.

When I run this locally, I see it starting agentbeat with each of the beats as sub-commands. However, each of these Beats is configured to do nothing, so we aren't testing as much as we should.

The example of the type of problem we want to catch is #39818, where agentbeat was silently not including an import that was included if you ran it directly as part of heartbeat.

The minimal reproduction for this is this heartbeat configuration which I'll put in heartbeat.yml:

heartbeat.monitors:
- type: browser
  enabled: true
  id: my-monitor
  schedule: '@every 10s'

output.console:
  hosts: ["localhost:9200"]
  enabled: true

Then if I revert 4c4b2f8 and run the following it reproduces:

git revert 4c4b2f899587cdb251b2c1d947395eb7664842bd
cd x-pack/agentbeat
mage build
./agentbeat heartbeat -c ./heartbeat.yml # Contains the heartbeat config above
Exiting: could not create monitor: factory could not create monitor: monitor type browser does not exist, valid types are [icmp synthetics/icmp tcp synthetics/tcp http synthetics/http]

What we need to do is to parse the list of inputs out of the spec files, and then create a config file for each of the Beats that includes all the inputs it can run, and then make sure that the Beat starts without exiting like this.

The config file for each Beat looks slightly different, see https://github.com/elastic/beats/blob/main/x-pack/filebeat/filebeat.yml for example compared to the heartbeat example above. Probably templating the config file would work for this.

Additionally, it would be nice if these tests were written as Go tests. Then the test report would be in the same format as all the other tests. At least a test case per beat, you could have sub-tests for each of the inputs.

@oakrizan oakrizan marked this pull request as draft October 25, 2024 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-8.x Automated backport to the 8.x branch with mergify backport-8.15 Automated backport to the 8.15 branch with mergify backport-8.16 Automated backport with mergify ci Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team Team:Ingest-EngProd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants