[Heartbeat] Add functional-ish tests in golang / fix bugs #32542

andrewvc · 2022-07-28T21:00:03Z

Fixes #31999

This PR turned out to be a bit of a monster. The main thing it does is add testing for functional 'scenarios', that is testing full heartbeat configs and the events they output going through all heartbeat specific code using the libbeat factory interface. This lets us do something sort of close to functional testing, where instead of using the heartbeat binary we actually use this factory interface. This gets us pretty close with a much simpler testing experience that's easier to debug and faster to run. It probably won't miss any error conditions from generating the full binary.

This also cleans up a bunch of linter issues in modified files, which is a bit distracting, things like missing error return values, shadowed variable names etc.

Finally, the new scenario test this adds, which checks that all events always contain a check group, revealed two bugs:

Synthetics test runs create an empty event at the start of each run (with no check group)
Any synth events prior to a journey start event would be missing a check group

This PR fixes this by:

Moving the check group generation code for synthexec up to the stream enricher, rather than the journey enricher.
Creating the check group UUIDs after journeys end, rather than waiting for them to start. The synthetics node agent can produce output before any journey starts

Why is it important?

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

elasticmachine · 2022-07-28T21:14:15Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-08-08T13:14:12.813+0000
Duration: 66 min 29 sec

Test stats 🧪

Test	Results
Failed	0
Passed	1648
Skipped	22
Total	1670

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

Fixes elastic#32082 by limiting browser jobs to 2

…check_group This PR is WIP and not ready for review, it doesn't do everything it should yet. In the situation where a browser job cannot start due to low memory (or anything that kills node / chrome too early) nothing will be visible in the Kibana UI due to the lack of a `check_group` field. This is because that field is generated upon encountering the first `journey/start` event. This PR fixes that by ensuring there's always a check group for single journey monitors.

…ng (elastic#32573) Co-authored-by: apmmachine <[email protected]>

…ng (elastic#32575) Co-authored-by: apmmachine <[email protected]>

…ic#32536) * update dependency elastic/go-structform from v0.0.9 to v0.0.10 Signed-off-by: Florian Lehner <[email protected]> Co-authored-by: Craig MacKenzie <[email protected]> Co-authored-by: Craig MacKenzie <[email protected]>

* add ulimit to debian containers * add changelog

andrewvc · 2022-08-04T00:21:32Z

@Mergifyio update

mergify · 2022-08-04T00:22:18Z

update

✅ Branch has been successfully updated

…ed-monitors

lucasfcosta

Overall, I didn't have any blocking comments. I tried really hard to see how mpx could become nil, but I couldn't see any way for it to become nil.

Also, it's quite annoying Jenkins is not showing you any of the logs 😢 I think we may have to use fmt here.

I've just started a repeat 10000 for dlv with a conditional breakpoint to see if I can catch this failure locally in a bit and debug.

heartbeat/beater/heartbeat.go

heartbeat/hbtestllext/isdefs.go

x-pack/heartbeat/monitors/browser/synthexec/synthexec.go

x-pack/heartbeat/scenarios/framework.go

lucasfcosta · 2022-08-04T16:28:13Z

x-pack/heartbeat/scenarios/framework.go

+	return mtr, err
+}
+
+func makeTestFactory() (factory *monitors.RunnerFactory, sched *scheduler.Scheduler, close func()) {


Given this is a function which takes 0 arguments, does it still need to be a "factory"? Can't we simply define the returned function straightaway and use it?

Oh, I guess this is confusing, it isn't a factory for making tests, it's a function that makes factories. Renamed to setupFactoryAndSched

lucasfcosta · 2022-08-04T16:30:31Z

x-pack/heartbeat/scenarios/framework.go

+		FirstStart:      time.Now(),
+		StartTime:       time.Now(),


NIT: perhaps we should save this right before so that we can avoid them being different if there's a small drift and we want to be ultra precise?

TBH, this info isn't actually used anywhere AFAIK, usually in tests all the fields are just left undefined. I also don't think any delta here would be meaningful, so in short, I'm 99% sure we're good.

lucasfcosta · 2022-08-04T16:36:06Z

x-pack/heartbeat/scenarios/framework.go

+	}
+
+	// make a pipeline
+	pipe := &monitors.MockPipeline{}


Love the way you used the mock pipeline to append items to an array! Really elegant reuse of the monitor code infra here.

andrewvc · 2022-08-04T18:36:00Z

I think I've addressed all your comments. I also added a fix in synthexec for a situation where the command can't be found. I think this may have some association with the weirdness we saw with browser monitors, but I also fixed the code to not try to run browser monitors on CI. We never should have been trying since the synthetics agent isn't present on CI.

I'm fine leaving these tests effectively disabled for now until such time as we fix that, at which point we can debug further. Esp. since locally they still run.

lucasfcosta

LGTM! Also cloned the repo and ran the functional tests locally to make sure they were running smoothly.

Although I have approved this, please do have a look at my comment about logging the os.Environ. I do think that's important to address and it would be great if you could check that comment and see if you think it's worth doing indeed.

All other comments are not blocking.

lucasfcosta · 2022-08-08T08:55:08Z

heartbeat/monitors/syncpipeclient.go

+}
+
+func (s SyncPipelineClientAdaptor) Wait() {
+	// intentionally blank, async pipelines should be empty


Wish we could have conditional types for these things like in TypeScript.

++ I wish so too

lucasfcosta · 2022-08-08T09:00:27Z

x-pack/heartbeat/monitors/browser/browser.go

@@ -27,6 +28,8 @@ func create(name string, cfg *config.C) (p plugin.Plugin, err error) {
 	// We don't want users running synthetics in environments that don't have the required GUI libraries etc, so we check
 	// this flag. When we're ready to support the many possible configurations of systems outside the docker environment
 	// we can remove this check.
+	ev := os.Environ()
+	logp.L().Info("EV %v", ev)


Do we want to keep this log line here? It feels to me we shouldn't log all the env. vars to stdout as that could leak information to logs.

No, good catch! I thought I'd pulled all of those

lucasfcosta · 2022-08-08T09:01:51Z

x-pack/heartbeat/monitors/browser/synthexec/synthexec.go

@@ -263,7 +272,7 @@ func runCmd(
 	// Close mpx after the process is done and all events have been sent / consumed
 	go func() {
 		err := <-cmdDone
-		jsonWriter.Close()
+		_ = jsonWriter.Close()


Any particular reason for us to do this assignment? Maybe it's my lack of familiarity with Go, but it doesn't seem it makes a difference in this case.

It makes intellij's linter happier. _ = means discard this. It's the only assignment that never uses := because nothing is actually assigned. See https://www.geeksforgeeks.org/what-is-blank-identifierunderscore-in-golang/

TBH, I'm not sure I should be changing this vs. changing my intellij settings but it's certainly not harmful.

to be clear, the linter complains about ignored error values, since go doesn't have exceptions

lucasfcosta · 2022-08-08T09:04:46Z

x-pack/heartbeat/monitors/browser/synthexec/synthexec.go

+		defer func() {
+			_ = jsonReader.Close()
+		}()


Can't we defer as we were doing previously?

I tried to do some research to understand the difference between this and the previous defer pattern and I couldn't find anything that would explain it.

I found this response on SO interesting but it actually advocates for the opposite pattern: directly deferring (although I can see the advantage of a lambda if we're doing any error handling for the deferred function.

Maybe you're doing it to evaluate jsonReader only when the function exits? I couldn't see why that would be helpful in this case though since we only assign to it with os.Pipe() at the beginning of the function?

If this is indeed intentional perhaps it would be worth leaving a comment as to why we do it.

Same story WRT the linter and errors

andrewvc · 2022-08-08T15:53:02Z

@Mergifyio backport 8.4

Fixes #31999 This PR turned out to be a bit of a monster. The main thing it does is add testing for functional 'scenarios', that is testing full heartbeat configs and the events they output going through all heartbeat specific code using the libbeat factory interface. This lets us do something sort of close to functional testing, where instead of using the heartbeat binary we actually use this factory interface. This gets us pretty close with a much simpler testing experience that's easier to debug and faster to run. It probably won't miss any error conditions from generating the full binary. This also cleans up a bunch of linter issues in modified files, which is a bit distracting, things like missing error return values, shadowed variable names etc. Finally, the new scenario test this adds, which checks that all events always contain a check group, revealed two bugs: Synthetics test runs create an empty event at the start of each run (with no check group) Any synth events prior to a journey start event would be missing a check group This PR fixes this by: Moving the check group generation code for synthexec up to the stream enricher, rather than the journey enricher. Creating the check group UUIDs after journeys end, rather than waiting for them to start. The synthetics node agent can produce output before any journey starts (cherry picked from commit 3fe6871)

mergify · 2022-08-08T15:53:43Z

backport 8.4

✅ Backports have been created

#32630 [Heartbeat] Add functional-ish tests in golang (backport #32542) has been created for branch 8.4

vigneshshanmugam · 2022-08-08T19:51:16Z

x-pack/heartbeat/monitors/browser/synthexec/enrich.go

@@ -206,6 +205,9 @@ func (je *journeyEnricher) createSummary(event *beat.Event) error {
 		return je.error
 	}

+	// create a new check group for the next journey
+	je.streamEnricher.checkGroup = makeUuid()


Question: Would check-group be unique if we run zip urls which could contain multiple journeys since we moved the generation to the stream enricher?

This code should still create multiple check groups for zip URLs. I don't think we want to assert this at the new functional tests though, since my hope is that zip urls will be gone soon enough.

Fixes #31999 This PR turned out to be a bit of a monster. The main thing it does is add testing for functional 'scenarios', that is testing full heartbeat configs and the events they output going through all heartbeat specific code using the libbeat factory interface. This lets us do something sort of close to functional testing, where instead of using the heartbeat binary we actually use this factory interface. This gets us pretty close with a much simpler testing experience that's easier to debug and faster to run. It probably won't miss any error conditions from generating the full binary. This also cleans up a bunch of linter issues in modified files, which is a bit distracting, things like missing error return values, shadowed variable names etc. Finally, the new scenario test this adds, which checks that all events always contain a check group, revealed two bugs: Synthetics test runs create an empty event at the start of each run (with no check group) Any synth events prior to a journey start event would be missing a check group This PR fixes this by: Moving the check group generation code for synthexec up to the stream enricher, rather than the journey enricher. Creating the check group UUIDs after journeys end, rather than waiting for them to start. The synthetics node agent can produce output before any journey starts (cherry picked from commit 3fe6871) Co-authored-by: Andrew Cholakian <[email protected]>

Fixes #31999 This PR turned out to be a bit of a monster. The main thing it does is add testing for functional 'scenarios', that is testing full heartbeat configs and the events they output going through all heartbeat specific code using the libbeat factory interface. This lets us do something sort of close to functional testing, where instead of using the heartbeat binary we actually use this factory interface. This gets us pretty close with a much simpler testing experience that's easier to debug and faster to run. It probably won't miss any error conditions from generating the full binary. This also cleans up a bunch of linter issues in modified files, which is a bit distracting, things like missing error return values, shadowed variable names etc. Finally, the new scenario test this adds, which checks that all events always contain a check group, revealed two bugs: Synthetics test runs create an empty event at the start of each run (with no check group) Any synth events prior to a journey start event would be missing a check group This PR fixes this by: Moving the check group generation code for synthexec up to the stream enricher, rather than the journey enricher. Creating the check group UUIDs after journeys end, rather than waiting for them to start. The synthetics node agent can produce output before any journey starts

andrewvc added 2 commits July 28, 2022 11:57

Checkpoint

66ee70b

FTest

101859e

andrewvc added Heartbeat Team:obs-ds-hosted-services Label for the Observability Hosted Services team labels Jul 28, 2022

andrewvc self-assigned this Jul 28, 2022

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jul 28, 2022

Add some basic scenarios

ed2e6bf

andrewvc mentioned this pull request Jul 29, 2022

Enhanced testing WIP #32540

Closed

6 tasks

andrewvc added 6 commits July 29, 2022 13:01

Fix mutual deps

e49383e

Fix tests

dff1244

Scenarios run at least

4d77c54

Fix broken tests, allow for build from windows hosts with WSL target

a602eac

[Heartbeat] Limit browser jobs to 2 by default

eb07c88

Fixes elastic#32082 by limiting browser jobs to 2

Merge remote-tracking branch 'origin/main' into refactor-test

481265c

andrewvc changed the title ~~[Heartbeat] Refactor tests WIP~~ [Heartbeat] Refactor tests Aug 2, 2022

andrewvc and others added 13 commits August 2, 2022 22:50

Checkpoint

8b61267

[Automation] Update elastic stack version to 8.5.0-c54c3404 for testi…

325b47e

…ng (elastic#32573) Co-authored-by: apmmachine <[email protected]>

[Automation] Update elastic stack version to 8.5.0-7dbc10f8 for testi…

44f5e31

…ng (elastic#32575) Co-authored-by: apmmachine <[email protected]>

Fix check group handling, refactor scenario framework

a3e73df

Fix check group handling, refactor scenario framework

1814ace

Fix license headers, make linter happy

cba2415

Fix some tests

8ea31a5

Fix some tests

be5a1f4

Add ulimit setting to debian containers in mage build (elastic#32580)

87d3930

* add ulimit to debian containers * add changelog

Remove strace stuff

5dce031

Make linter happy

ec851f1

Merge branch 'main' into refactor-test

b0acfe6

andrewvc added 8 commits August 3, 2022 20:07

Try to debug test failures

cf2688f

Try to debug test failures

9fc2901

fmt

7584944

Only run browser and ICMP tests when available

4528443

Merge remote-tracking branch 'andrewvc/refactor-test' into fix-disabl…

be138fe

…ed-monitors

Improve debugging

a5bad89

Improve debugging

6074b6d

Fix scenario capability filtering

5ce9bb3

lucasfcosta reviewed Aug 4, 2022

View reviewed changes

andrewvc added 2 commits August 4, 2022 13:24

Cleanup based on PR feedback etc.

0c59599

Better naming

267743a

andrewvc mentioned this pull request Aug 4, 2022

[Heartbeat] Update copy in info message to be less confusing #32581

Closed

2 tasks

lucasfcosta approved these changes Aug 8, 2022

View reviewed changes

Incoporate PR feedback

7b50987

andrewvc merged commit 3fe6871 into elastic:main Aug 8, 2022

andrewvc deleted the refactor-test branch August 8, 2022 15:52

andrewvc added the v8.4.0 label Aug 8, 2022

andrewvc changed the title ~~[Heartbeat] Add functional-ish tests in golang~~ [Heartbeat] Add functional-ish tests in golang / fix bugs Aug 8, 2022

mergify bot mentioned this pull request Aug 8, 2022

[Heartbeat] Add functional-ish tests in golang (backport #32542) #32630

Merged

vigneshshanmugam reviewed Aug 8, 2022

View reviewed changes

v1v mentioned this pull request Aug 16, 2022

ci: enable synthetics tests for heartbeats #32710

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Heartbeat] Add functional-ish tests in golang / fix bugs #32542

[Heartbeat] Add functional-ish tests in golang / fix bugs #32542

andrewvc commented Jul 28, 2022 •

edited

Loading

elasticmachine commented Jul 28, 2022 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

andrewvc commented Aug 4, 2022

mergify bot commented Aug 4, 2022

lucasfcosta left a comment

lucasfcosta Aug 4, 2022

andrewvc Aug 4, 2022

lucasfcosta Aug 4, 2022

andrewvc Aug 4, 2022

lucasfcosta Aug 4, 2022

andrewvc commented Aug 4, 2022

lucasfcosta left a comment

lucasfcosta Aug 8, 2022

andrewvc Aug 8, 2022

lucasfcosta Aug 8, 2022

andrewvc Aug 8, 2022

lucasfcosta Aug 8, 2022

andrewvc Aug 8, 2022

andrewvc Aug 8, 2022

lucasfcosta Aug 8, 2022

andrewvc Aug 8, 2022

andrewvc commented Aug 8, 2022

mergify bot commented Aug 8, 2022

vigneshshanmugam Aug 8, 2022

andrewvc Aug 9, 2022

[Heartbeat] Add functional-ish tests in golang / fix bugs #32542

[Heartbeat] Add functional-ish tests in golang / fix bugs #32542

Conversation

andrewvc commented Jul 28, 2022 • edited Loading

Why is it important?

Checklist

How to test this PR locally

elasticmachine commented Jul 28, 2022 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

andrewvc commented Aug 4, 2022

mergify bot commented Aug 4, 2022

✅ Branch has been successfully updated

lucasfcosta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewvc commented Aug 4, 2022

lucasfcosta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewvc commented Aug 8, 2022

mergify bot commented Aug 8, 2022

✅ Backports have been created

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewvc commented Jul 28, 2022 •

edited

Loading

elasticmachine commented Jul 28, 2022 •

edited by jenkins-beats-ci bot

Loading