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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 10 commits into from
Sep 29, 2023
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
dfa3651
Fix the summarizer tests
andrewvc Sep 29, 2023
949343d
tests are now red
andrewvc Sep 29, 2023
088c896
Refine tests and fix retries
andrewvc Sep 29, 2023
b9c0dc1
Revert unnecessary summarizer test changes
andrewvc Sep 29, 2023
01181f5
Various fixes and cleanups
andrewvc Sep 29, 2023
7bc02fa
Merge branch 'main' into sumretrfix
andrewvc Sep 29, 2023
017cea7
Make linter happy
andrewvc Sep 29, 2023
a51b909
Merge remote-tracking branch 'andrewvc/sumretrfix' into sumretrfix
andrewvc Sep 29, 2023
f0ca003
Improve logging
andrewvc Sep 29, 2023
890a4dc
PR feedback
andrewvc Sep 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 13 additions & 11 deletions heartbeat/monitors/wrappers/summarizer/plugstatestat.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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
}

Copy link
Contributor Author

@andrewvc andrewvc Sep 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

if ssp.cssp.js.Down == 0 {
// Browsers don't have a prior increment of this, so set it to some
// non-zero value
ssp.cssp.js.Up = 1
ssp.cssp.js.Status = monitorstate.StatusUp
}

res := ssp.cssp.BeforeSummary(event)

// Browsers don't set this prior, so we set this here, as opposed to lightweight monitors
_, _ = event.PutValue("monitor.status", string(ssp.cssp.js.Status))

_, _ = event.PutValue("synthetics", mapstr.M{"type": "heartbeat/summary"})
vigneshshanmugam marked this conversation as resolved.
Show resolved Hide resolved
return res
}

func (ssp *BrowserStateStatusPlugin) BeforeRetry() {
// noop
ssp.cssp.BeforeRetry()
}

// LightweightStateStatusPlugin encapsulates the writing of the primary fields used by the summary,
Expand Down Expand Up @@ -108,7 +112,7 @@ func (ssp *LightweightStateStatusPlugin) BeforeSummary(event *beat.Event) Before
}

func (ssp *LightweightStateStatusPlugin) BeforeRetry() {
// noop
ssp.cssp.BeforeRetry()
}

type commonSSP struct {
Expand Down Expand Up @@ -162,15 +166,8 @@ func (ssp *commonSSP) BeforeSummary(event *beat.Event) BeforeSummaryActions {
"summary": &jsCopy,
"state": ms,
}
if ssp.sf.Type == "browser" {
fields["synthetics"] = mapstr.M{"type": "heartbeat/summary"}
}
eventext.MergeEventFields(event, fields)

if retry {
// 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)
Copy link
Member

Choose a reason for hiding this comment

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

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)


Expand All @@ -180,3 +177,8 @@ func (ssp *commonSSP) BeforeSummary(event *beat.Event) BeforeSummaryActions {

return 0
}

func (ssp *commonSSP) BeforeRetry() {
// mutate the js into the state for the next attempt
ssp.js.BumpAttempt()
}
39 changes: 27 additions & 12 deletions x-pack/heartbeat/scenarios/basics_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"github.com/elastic/beats/v7/heartbeat/monitors/wrappers/monitorstate"
"github.com/elastic/beats/v7/heartbeat/monitors/wrappers/summarizer/jobsummary"
"github.com/elastic/beats/v7/heartbeat/monitors/wrappers/summarizer/summarizertesthelper"
"github.com/elastic/beats/v7/libbeat/beat"
"github.com/elastic/beats/v7/x-pack/heartbeat/scenarios/framework"
)

Expand Down Expand Up @@ -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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We now run these tests with both 1 and 2 max_attempts

all := mtr.Events()
lastEvent, firstEvents := all[len(all)-1], all[:len(all)-1]
lastEvent := all[len(all)-1]
testslike.Test(t,
SummaryValidatorForStatus(mtr.Meta.Status),
lastEvent.Fields)

for _, e := range firstEvents {
summary, _ := e.GetValue("summary")
require.Nil(t, summary)
}
requireOneSummaryPerAttempt(t, all)
})
}

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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We now run these tests with both 1 and 2 max_attempts

all := mtr.Events()
lastEvent, firstEvents := all[len(all)-1], all[:len(all)-1]
lastEvent := all[len(all)-1]

testslike.Test(t,
lookslike.Compose(
Expand All @@ -126,13 +124,30 @@ func TestBrowserSummaries(t *testing.T) {
),
lastEvent.Fields)

for _, e := range firstEvents {
summary, _ := e.GetValue("summary")
require.Nil(t, summary)
}
monStatus, _ := lastEvent.GetValue("monitor.status")
summaryIface, _ := lastEvent.GetValue("summary")
summary := summaryIface.(*jobsummary.JobSummary)
require.Equal(t, string(summary.Status), monStatus, "expected summary status and mon status to be equal in event: %v", lastEvent.Fields)

requireOneSummaryPerAttempt(t, all)

})
}

func requireOneSummaryPerAttempt(t *testing.T, events []*beat.Event) {
attemptCounter := uint16(1)
// ensure we only have one summary per attempt
for _, e := range events {
summaryIface, _ := e.GetValue("summary")
if summaryIface != nil {
summary := summaryIface.(*jobsummary.JobSummary)
require.Equal(t, attemptCounter, summary.Attempt)
require.LessOrEqual(t, summary.Attempt, summary.MaxAttempts)
attemptCounter++
}
}
}

func TestRunFromOverride(t *testing.T) {
t.Parallel()
scenarioDB.RunAllWithATwist(t, TwistAddRunFrom, func(t *testing.T, mtr *framework.MonitorTestRun, err error) {
Expand Down
2 changes: 1 addition & 1 deletion x-pack/heartbeat/scenarios/browserscenarios.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func init() {
framework.Scenario{
Name: "failing-browser",
Type: "browser",
Tags: []string{"browser", "browser-inline", "down"},
Tags: []string{"browser", "browser-inline", "down", "browser-down"},
Runner: func(t *testing.T) (config mapstr.M, meta framework.ScenarioRunMeta, close func(), err error) {
err = os.Setenv("ELASTIC_SYNTHETICS_CAPABLE", "true")
if err != nil {
Expand Down
6 changes: 6 additions & 0 deletions x-pack/heartbeat/scenarios/framework/framework.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,12 @@ func (sdb *ScenarioDB) RunTagWithATwist(t *testing.T, tagName string, twist *Twi
}
}

func (sdb *ScenarioDB) RunTagWithSeparateTwists(t *testing.T, tagName string, twists []*Twist, callback func(*testing.T, *MonitorTestRun, error)) {
for _, twist := range twists {
sdb.RunTagWithATwist(t, tagName, twist, callback)
}
}

type MonitorTestRun struct {
StdFields stdfields.StdMonitorFields
Meta ScenarioRunMeta
Expand Down
3 changes: 3 additions & 0 deletions x-pack/heartbeat/scenarios/twists.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@
})
}

// Test with both one and two twists

Check failure on line 39 in x-pack/heartbeat/scenarios/twists.go

View workflow job for this annotation

GitHub Actions / lint (linux) 

ST1022: comment on exported var StdAttemptTwists should be of the form "StdAttemptTwists ..." (stylecheck)
var StdAttemptTwists = []*framework.Twist{TwistMaxAttempts(1), TwistMaxAttempts(2)}

func TwistMaxAttempts(maxAttempts int) *framework.Twist {
return framework.MakeTwist(fmt.Sprintf("run with %d max_attempts", maxAttempts), func(s framework.Scenario) framework.Scenario {
s.Tags = append(s.Tags, "retry")
Expand Down
Loading