[Heartbeat] Fix missing monitor status on 1 of 2 attempts #36704

andrewvc · 2023-09-29T01:14:46Z

Proposed commit message

[Heartbeat] Fix missing monitor.status value in initial attempt where max_attempts > 2. Introduced in #36623 adding tests to the scenario runner as well.

Original cause was this PR #36519 that did not produce the correct monitor.status: down when the monitor is retried with the second attempt.

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

Use the following monitor config with JSON output to console enabled output.console: ~

- type: browser
  id: test-hb
  enabled: true
  name: Test HB dev
  schedule: '@every 1m'
  screenshots: "off"
  max_attempts: 2
  source:
    inline:
      script: |-
        step("load homepage", async ) => {

        });

# Note the broken output here on the main branch
vscode ➜ /workspaces/beats/x-pack/heartbeat (main) $ ELASTIC_SYNTHETICS_CAPABLE=true ./heartbeat | grep heartbeat/summary | jq .event.type,.monitor.status,.summary.status,.summary.attempt
"heartbeat/summary"
""
"down"
1
"heartbeat/summary"
"down"
"down"
2

vscode ➜ /workspaces/beats/x-pack/heartbeat (main) $ git checkout sumretrfix
Switched to branch 'sumretrfix'
vscode ➜ /workspaces/beats/x-pack/heartbeat (sumretrfix) $ mage build
>> build: Building heartbeat
# Note the correct output with a monitor.status value of "down"
vscode ➜ /workspaces/beats/x-pack/heartbeat (sumretrfix) $ ELASTIC_SYNTHETICS_CAPABLE=true ./heartbeat | grep heartbeat/summary | jq .event.type,.monitor.status,.summary.status,.summary.attempt
"heartbeat/summary"
"down"
"down"
1
"heartbeat/summary"
"down"
"down"
2

Related issues

Use cases

Screenshots

Logs

elasticmachine · 2023-09-29T01:14:50Z

Pinging @elastic/uptime (Team:Uptime)

mergify · 2023-09-29T01:15:22Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @andrewvc? 🙏.
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-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

elasticmachine · 2023-09-29T01:20:56Z

💚 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: 2023-09-29T13:30:42.297+0000
Duration: 51 min 4 sec

Test stats 🧪

Test	Results
Failed	0
Passed	2570
Skipped	28
Total	2598

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the 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!)

heartbeat/monitors/wrappers/summarizer/plugstatestat.go

andrewvc · 2023-09-29T01:28:35Z

x-pack/heartbeat/scenarios/basics_test.go

@@ -99,25 +100,22 @@ func TestLightweightUrls(t *testing.T) {

 func TestLightweightSummaries(t *testing.T) {
 	t.Parallel()
-	scenarioDB.RunTag(t, "lightweight", func(t *testing.T, mtr *framework.MonitorTestRun, err error) {
+	scenarioDB.RunTagWithSeparateTwists(t, "lightweight", StdAttemptTwists, func(t *testing.T, mtr *framework.MonitorTestRun, err error) {


We now run these tests with both 1 and 2 max_attempts

andrewvc · 2023-09-29T01:28:40Z

x-pack/heartbeat/scenarios/basics_test.go

 	})
 }

 func TestBrowserSummaries(t *testing.T) {
 	t.Parallel()
-	scenarioDB.RunTag(t, "browser", func(t *testing.T, mtr *framework.MonitorTestRun, err error) {
+	scenarioDB.RunTagWithSeparateTwists(t, "browser", StdAttemptTwists, func(t *testing.T, mtr *framework.MonitorTestRun, err error) {


We now run these tests with both 1 and 2 max_attempts

vigneshshanmugam

LGTM.

vigneshshanmugam · 2023-09-29T03:58:56Z

heartbeat/monitors/wrappers/summarizer/plugstatestat.go

@@ -56,20 +56,24 @@ func (ssp *BrowserStateStatusPlugin) EachEvent(event *beat.Event, jobErr error)
 }

 func (ssp *BrowserStateStatusPlugin) BeforeSummary(event *beat.Event) BeforeSummaryActions {
+	ssp.cssp.js.Status = monitorstate.StatusDown


If we already set the Status here, it can be safely removed from the common BeforeSummary -

beats/heartbeat/monitors/wrappers/summarizer/plugstatestat.go

Lines 144 to 148 in a51b909

if ssp.js.Down > 0 {

ssp.js.Status = monitorstate.StatusDown

} else {

ssp.js.Status = monitorstate.StatusUp

}

I'll do it the other way and leave that in common since we still need that for common

vigneshshanmugam · 2023-09-29T04:13:04Z

heartbeat/monitors/wrappers/summarizer/plugstatestat.go

-		// mutate the js into the state for the next attempt
-		ssp.js.BumpAttempt()
-	}
+	eventext.MergeEventFields(event, fields)

 	logp.L().Debugf("attempt info: %v == %v && %d < %d", ssp.js.Status, lastStatus, ssp.js.Attempt, ssp.js.MaxAttempts)


Can you add last vs current status and attempts. Felt more descriptive -

logp.L().Debugf("attempt info: current(%v) == lastStatus(%v) && attempts(%d < %d)", ssp.js.Status, lastStatus, ssp.js.Attempt, ssp.js.MaxAttempts)

) [Heartbeat] Fix missing monitor.status value in initial attempt where max_attempts > 2. Introduced in elastic#36623 adding tests to the scenario runner as well. Original cause was this PR elastic#36519 that did not produce the correct monitor.status: down when the monitor is retried with the second attempt.

andrewvc added 3 commits September 29, 2023 00:38

Fix the summarizer tests

dfa3651

tests are now red

949343d

Refine tests and fix retries

088c896

andrewvc added bug Team:obs-ds-hosted-services Label for the Observability Hosted Services team v8.11.0 labels Sep 29, 2023

andrewvc requested a review from a team as a code owner September 29, 2023 01:14

andrewvc self-assigned this Sep 29, 2023

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 Sep 29, 2023

andrewvc added Heartbeat needs_team Indicates that the issue/PR needs a Team:* label labels Sep 29, 2023

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Sep 29, 2023

Revert unnecessary summarizer test changes

b9c0dc1

Various fixes and cleanups

01181f5

andrewvc commented Sep 29, 2023

View reviewed changes

heartbeat/monitors/wrappers/summarizer/plugstatestat.go Show resolved Hide resolved

andrewvc commented Sep 29, 2023

View reviewed changes

andrewvc requested a review from vigneshshanmugam September 29, 2023 01:28

andrewvc mentioned this pull request Sep 29, 2023

[Heartbeat]: fix status field when monitors are down #36623

Closed

andrewvc and others added 3 commits September 28, 2023 20:36

Merge branch 'main' into sumretrfix

7bc02fa

Make linter happy

017cea7

Merge remote-tracking branch 'andrewvc/sumretrfix' into sumretrfix

a51b909

vigneshshanmugam approved these changes Sep 29, 2023

View reviewed changes

andrewvc added 2 commits September 29, 2023 13:02

Improve logging

f0ca003

PR feedback

890a4dc

andrewvc changed the title ~~Sumretrfix~~ [Heartbeat] Fix missing monitor status on 1 of 2 attempts Sep 29, 2023

andrewvc merged commit 9e31636 into elastic:main Sep 29, 2023
25 checks passed

andrewvc deleted the sumretrfix branch September 29, 2023 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Heartbeat] Fix missing monitor status on 1 of 2 attempts #36704

[Heartbeat] Fix missing monitor status on 1 of 2 attempts #36704

andrewvc commented Sep 29, 2023 •

edited

Loading

elasticmachine commented Sep 29, 2023

mergify bot commented Sep 29, 2023

elasticmachine commented Sep 29, 2023 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

andrewvc Sep 29, 2023

andrewvc Sep 29, 2023

vigneshshanmugam left a comment

vigneshshanmugam Sep 29, 2023

andrewvc Sep 29, 2023 •

edited

Loading

vigneshshanmugam Sep 29, 2023

	if ssp.js.Down > 0 {
	ssp.js.Status = monitorstate.StatusDown
	} else {
	ssp.js.Status = monitorstate.StatusUp
	}

[Heartbeat] Fix missing monitor status on 1 of 2 attempts #36704

[Heartbeat] Fix missing monitor status on 1 of 2 attempts #36704

Conversation

andrewvc commented Sep 29, 2023 • edited Loading

Proposed commit message

Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

elasticmachine commented Sep 29, 2023

mergify bot commented Sep 29, 2023

elasticmachine commented Sep 29, 2023 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

andrewvc Sep 29, 2023

Choose a reason for hiding this comment

andrewvc Sep 29, 2023

Choose a reason for hiding this comment

vigneshshanmugam left a comment

Choose a reason for hiding this comment

vigneshshanmugam Sep 29, 2023

Choose a reason for hiding this comment

andrewvc Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

vigneshshanmugam Sep 29, 2023

Choose a reason for hiding this comment

andrewvc commented Sep 29, 2023 •

edited

Loading

elasticmachine commented Sep 29, 2023 •

edited by jenkins-beats-ci bot

Loading

andrewvc Sep 29, 2023 •

edited

Loading