[Heartbeat]: catch all error handler for browser jobs by vigneshshanmugam · Pull Request #30013 · elastic/beats

vigneshshanmugam · 2022-01-26T07:13:58Z

fix Inline monitor syntax errors should send errors to ES #29897
fix [Heartbeat] Stop appending - inline to monitors #29505
Consume all messages both from stderr and stdout from the synthetics runner and add them to synthetics.payload.message. Anything written during the broken journey will be used by Uptime UI for showing debug messages.
Introduce suite.* details for all the suite jobs.
Wrap all the synthetics browser releated fields in the Synthetics Enricher and drop some of the logic in the global wrapper to make things easier.
Gets rid of <monitior>-inline inline suffix from inline monitors.

mergify · 2022-01-26T07:14:30Z

This pull request does not have a backport label. Could you fix it @vigneshshanmugam? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

elasticmachine · 2022-01-26T07:44:57Z

💚 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

Reason: null
Start Time: 2022-02-01T01:01:53.556+0000
Duration: 77 min 57 sec
Commit: c0b4cc0

Test stats 🧪

Test	Results
Failed	0
Passed	3307
Skipped	71
Total	3378

💚 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!)

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

andrewvc

Leaving a partial review, still reading this completely, lots of good stuff here!

heartbeat/monitors/wrappers/wrappers.go

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

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

andrewvc

Can we add a test for the behavior of suites when no journey is run? This would test that if the invoked command fails we still generate events with suite metadata etc. even though no journeys are present.

andrewvc · 2022-01-28T19:55:21Z

Actually, disregard my last comment. In enrich_test.go we can simulate any output we like from a fake invocation of the synthetics command, we should just add a result with something bad happening instead of journeys running.

elasticmachine · 2022-01-29T01:37:31Z

Pinging @elastic/uptime (Team:Uptime)

andrewvc

Generally looking good! I want to run some local tests. Also, it looks like you missed my feedback on synthexec.go, when you get a chance could you respond to it?

Nothing there is major, great work on a complex PR!

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

heartbeat/monitors/wrappers/wrappers.go

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

andrewvc · 2022-01-31T23:36:32Z

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

+				},
+			},
+			func(t *testing.T, e *beat.Event, je *journeyEnricher) {
+				v := lookslike.MustCompile(common.MapStr{


Would there be any value in also testing the monitor.* values that are set here? Also, as a note, you can use lookslike.Strict to ensure that there are no extra fields if that's appropriate. It just wraps the outer lookslike.MustCompile, but you do have to define every field.

Not sure if there would be any value, I added it to denote the case when there are no journeys ran and this was the only message from console.

andrewvc · 2022-01-31T23:39:20Z

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

-		e.synthEvents <- se
-	}
+
+	e.synthEvents <- se


There's a weird thing to consider, which is errors between journeys.

We could still need to check for journey/end to account for errors that might happen between journeys. I think we can just set e.currentJourney.store(false) again, no? Then it just gets reported as an error on the suite.

Our long term goal is for heartbeat to really only execute one journey per monitor (with discovery mode) so I'm OK with not handling this for the moment and just tacking those errors on the previous journey. Also, I don't know what an error between journeys would even be.

I am not sure if there would be error between journeys, But with this change we would still get stderr messages on the suite level, wouldnt that be enough to figure out any errors?

vigneshshanmugam · 2022-01-31T23:54:42Z

Also, it looks like you missed my feedback on synthexec.go, when you get a chance could you respond to it?

Oops, can you point me to that one? Is it about the naming stdSuiteFields? I have already fixed that if thats the case.

andrewvc

LGTM, great work!

andrewvc · 2022-02-01T03:59:19Z

I'm wondering, should we backport this to 7.17.0 , I would say that this is mostly a bugfix, the extra suite stuff won't affect 7.17.0 otherwise

vigneshshanmugam · 2022-02-01T04:51:58Z

I'm wondering, should we backport this to 7.17.0 , I would say that this is mostly a bugfix, the extra suite stuff won't affect 7.17.0 otherwise

Yeah ++ on back porting it.

* [Heartbeat]: catch all error handler for browser jobs * fix wrapper tests * add tests and consume all events * address review and improve test (cherry picked from commit 267f8cb)

* [Heartbeat]: catch all error handler for browser jobs * fix wrapper tests * add tests and consume all events * address review and improve test (cherry picked from commit 267f8cb) Co-authored-by: Vignesh Shanmugam <vignesh.shanmugam22@gmail.com>

[Heartbeat]: catch all error handler for browser jobs

b6c95f7

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jan 26, 2022

vigneshshanmugam added the Team:obs-ds-hosted-services Label for the Observability Hosted Services team label Jan 26, 2022

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jan 26, 2022

mergify bot added the backport-skip Skip notification from the automated backport with mergify label Jan 26, 2022

vigneshshanmugam added the v8.1.0 label Jan 26, 2022

fix wrapper tests

d145c0d

vigneshshanmugam commented Jan 26, 2022

View reviewed changes

x-pack/heartbeat/monitors/browser/synthexec/execmultiplexer.go Show resolved Hide resolved

andrewvc reviewed Jan 27, 2022

View reviewed changes

andrewvc reviewed Jan 28, 2022

View reviewed changes

add tests and consume all events

0d6f438

vigneshshanmugam marked this pull request as ready for review January 29, 2022 01:37

vigneshshanmugam requested a review from a team as a code owner January 29, 2022 01:37

vigneshshanmugam added backport-v8.0.0 Automated backport with mergify and removed backport-skip Skip notification from the automated backport with mergify labels Jan 29, 2022

vigneshshanmugam requested a review from andrewvc January 29, 2022 04:58

vigneshshanmugam mentioned this pull request Jan 31, 2022

[Heartbeat] Better handling of subfolders in package.json #30119

Closed

andrewvc reviewed Jan 31, 2022

View reviewed changes

address review and improve test

c0b4cc0

vigneshshanmugam requested a review from andrewvc February 1, 2022 01:02

andrewvc assigned vigneshshanmugam Feb 1, 2022

andrewvc approved these changes Feb 1, 2022

View reviewed changes

vigneshshanmugam added the backport-7.17 Automated backport to the 7.17 branch with mergify label Feb 1, 2022

vigneshshanmugam added the bug label Feb 1, 2022

vigneshshanmugam merged commit 267f8cb into elastic:master Feb 1, 2022

mergify bot mentioned this pull request Feb 1, 2022

[7.17](backport #30013) [Heartbeat]: catch all error handler for browser jobs #30126

Merged

mergify bot mentioned this pull request Feb 1, 2022

[8.0](backport #30013) [Heartbeat]: catch all error handler for browser jobs #30127

Merged

vigneshshanmugam added the backport-v8.1.0 Automated backport with mergify label Feb 3, 2022

elastic deleted a comment from mergify bot Feb 3, 2022

This comment was marked as resolved.

Sign in to view

Conversation

vigneshshanmugam commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergify bot commented Jan 26, 2022

Uh oh!

elasticmachine commented Jan 26, 2022 • edited by jenkins-beats-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

Uh oh!

Uh oh!

andrewvc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewvc left a comment

Choose a reason for hiding this comment

Uh oh!

andrewvc commented Jan 28, 2022

Uh oh!

elasticmachine commented Jan 29, 2022

Uh oh!

andrewvc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewvc Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

vigneshshanmugam Feb 1, 2022

Choose a reason for hiding this comment

Uh oh!

andrewvc Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

vigneshshanmugam Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

vigneshshanmugam commented Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewvc left a comment

Choose a reason for hiding this comment

Uh oh!

andrewvc commented Feb 1, 2022

Uh oh!

vigneshshanmugam commented Feb 1, 2022

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vigneshshanmugam commented Jan 26, 2022 •

edited

Loading

elasticmachine commented Jan 26, 2022 •

edited by jenkins-beats-ci bot

Loading

vigneshshanmugam commented Jan 31, 2022 •

edited

Loading