[Heartbeat] Adjust State loader to only retry for failed requests and not for 4xx #37981

devcorpio · 2024-02-12T15:31:47Z

Proposed commit message

Closes #37424

From now on, the state loader will just retry for failed requests (such as a 500 status code) and not for 4xx.

Additional context

If the query is valid and returns zero results, there will not be any retry, this was working this way already
If the API key is invalid, the query will fail returning a 4xx error (this was working this way already), as just can see in the screenshot below:

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.

devcorpio · 2024-02-12T15:33:39Z

heartbeat/monitors/wrappers/monitorstate/esloader.go

@@ -94,7 +103,8 @@ func MakeESLoader(esc *eslegclient.Connection, indexPattern string, beatLocation
 		sh := stateHits{}
 		err = json.Unmarshal(body, &sh)
 		if err != nil {
-			return nil, fmt.Errorf("could not unmarshal state hits for %s: %w", sf.ID, err)
+			errMsg := fmt.Errorf("could not unmarshal state hits for %s: %w", sf.ID, err).Error()
+			return nil, LoaderError{Message: errMsg, Retry: true}


I set the retry property to true to avoid changing the behaviour. But now that we are revising this... do we want to retry if there is an error while marshaling?

Even though this shouldn't happen in practice, I dont think there is any value in doing retries for malformed data.

devcorpio · 2024-02-12T15:35:14Z

heartbeat/monitors/wrappers/monitorstate/esloader_test.go

 	etc := &esTestContext{
 		namespace: namespace.String(),
 		esc:       esc,
-		loader:    IntegESLoader(t, fmt.Sprintf("synthetics-*-%s", namespace.String()), location),
+		ec:        ec,


change made to make the HTTP API obj "fakeable" as you can see in esloader_test.go

elasticmachine · 2024-02-12T15:39:13Z

Pinging @elastic/obs-ds-hosted-services (Team:obs-ds-hosted-services)

elasticmachine · 2024-02-12T15:40:29Z

💚 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

Duration: 50 min 58 sec

❕ Flaky test report

No test was executed to be analysed.

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

devcorpio · 2024-02-12T15:42:33Z

heartbeat/monitors/wrappers/monitorstate/tracker_test.go

+		expectedCalls int
+	}{
+		{
+			"should retry 3 times when fails with retryable error",


this test lasts 5 seconds given that time.Sleep is executed for every retry. Let me know if you want me to make this parameterizable or if you believe it's something negligible.

Maybe we should make it configurable.

CHANGELOG.next.asciidoc

vigneshshanmugam

Couple of needed changes

Avoid retry on document not found - 404 - ES wont return status: 404, it will be on the response - [Heartbeat] Adjust State loader to only retry for failed requests and not for 4xx #37424 (comment). Please confirm if that is the case.
Avoid retries on Context cancellations / DeadlineExceeded - Timeouts

net/http: request canceled (Client.Timeout exceeded while awaiting headers)

Please do verify what happens for 410 - Gone, if the resource was deleted.

vigneshshanmugam · 2024-02-12T20:06:04Z

heartbeat/monitors/wrappers/monitorstate/esloader.go

@@ -94,7 +103,8 @@ func MakeESLoader(esc *eslegclient.Connection, indexPattern string, beatLocation
 		sh := stateHits{}
 		err = json.Unmarshal(body, &sh)
 		if err != nil {
-			return nil, fmt.Errorf("could not unmarshal state hits for %s: %w", sf.ID, err)
+			errMsg := fmt.Errorf("could not unmarshal state hits for %s: %w", sf.ID, err).Error()
+			return nil, LoaderError{Message: errMsg, Retry: true}


Even though this shouldn't happen in practice, I dont think there is any value in doing retries for malformed data.

vigneshshanmugam · 2024-02-12T20:07:22Z

heartbeat/monitors/wrappers/monitorstate/esloader.go

@@ -94,7 +103,8 @@ func MakeESLoader(esc *eslegclient.Connection, indexPattern string, beatLocation
 		sh := stateHits{}
 		err = json.Unmarshal(body, &sh)
 		if err != nil {
-			return nil, fmt.Errorf("could not unmarshal state hits for %s: %w", sf.ID, err)
+			errMsg := fmt.Errorf("could not unmarshal state hits for %s: %w", sf.ID, err).Error()


ques: why was .Error called here?

Before the change, fmt.Errorf was getting an error as an argument. This means that's wrapping an error.

Given that the new logic creates a new error type (LoaderError), I wanted to ensure the error message was identically when logging it.

Having said that, after thinking about this further, I will change this logic a bit:

Rather than:

type LoaderError struct { Message string Retry bool }

I'll create:

type LoaderError struct { err error Retry bool }

and will pass an error instance to LoaderError. This way, we move that concern to the error obj/struct, something like this:

func (e LoaderError) Error() string { return e.err.Error() }

which partially matches with how other places of heartbeat resolves similar scenarios:

beats/heartbeat/reason/reason.go

Line 50 in 85e4e46

func (e ValidateError) Error() string { return e.err.Error() }

I will add the commit shortly.

will check why the log appears "weird":

vigneshshanmugam · 2024-02-12T20:13:58Z

heartbeat/monitors/wrappers/monitorstate/esloader.go

@@ -107,3 +117,11 @@ func MakeESLoader(esc *eslegclient.Connection, indexPattern string, beatLocation
 		return state, nil
 	}
 }
+
+func shouldRetry(status int) bool {
+	if status > 200 && status <= 499 {


nit: status >= 500 - true else false

vigneshshanmugam · 2024-02-12T20:17:49Z

heartbeat/monitors/wrappers/monitorstate/tracker_test.go

+		expectedCalls int
+	}{
+		{
+			"should retry 3 times when fails with retryable error",


Maybe we should make it configurable.

devcorpio · 2024-02-22T17:36:05Z

Avoid retry on document not found - 404 - ES wont return status: 404, it will be on the response

if a document is not found, the response contains 0 hits, and that is not treated as a situation that requires a retry. Hence, there is no retry:

if the ES index is not found, it will return 404. There will not be a retry either.

Avoid retries on Context cancellations / DeadlineExceeded - Timeouts

If there is a timeout, the status code is 0. There will not be a retry either.

Please do verify what happens for 410 - Gone, if the resource was deleted.

Forced this case with Charles Proxy, since I was unable to reproduce a "normal" 410, I was always getting the 0 hits thingy. Anyway, no retry either.

Note: bear in mind that from now on, we will only retry if there is a status code >= 500 (also reproduced this with Charles, added breaking point to a particular url request and aborting the request, great tool btw)

mergify · 2024-02-22T19:41:42Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @devcorpio? 🙏.
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

vigneshshanmugam

LGTM except for the wrapped error formatting.

devcorpio · 2024-02-23T10:42:35Z

LGTM except for the wrapped error formatting.

It turns out that if we want to log the error as a string we need to use "%v". It also seems that we had the very format issue before creating the new error as you can see here:

Replacing it with %v makes things show with the expected format:

🎉

vigneshshanmugam

LGTM

vigneshshanmugam · 2024-02-23T18:27:31Z

heartbeat/monitors/wrappers/monitorstate/tracker.go

@@ -111,7 +111,7 @@ func (t *Tracker) GetCurrentState(sf stdfields.StdMonitorFields, rc RetryConfig)
 		}
 		var loaderError LoaderError
 		if errors.As(err, &loaderError) && !loaderError.Retry {
-			logp.L().Warnf("could not load last externally recorded state: %w", err)
+			logp.L().Warnf("could not load last externally recorded state: %v", loaderError)


I guess if we want to use %w, we would need to unwrap errors.Unwrap. Not super sure. But %v should called the Error() method.

elasticmachine · 2024-03-04T09:17:46Z

💚 Build Succeeded

Buildkite Build
Commit: 0b8a902

History

💚 Build #2907 succeeded 70a7d46
💚 Build #2821 succeeded 257381b
💚 Build #2808 succeeded c0e559c
💚 Build #2807 succeeded 3d0aba5
💚 Build #2806 succeeded 480a6a1
💚 Build #2785 succeeded 5a0cf94

cc @devcorpio

elasticmachine · 2024-03-04T09:18:01Z

💚 Build Succeeded

Buildkite Build
Commit: 0b8a902

History

💚 Build #1700 succeeded 70a7d46
💚 Build #1614 succeeded 257381b
💚 Build #1601 succeeded c0e559c
💚 Build #1600 succeeded 3d0aba5
💚 Build #1599 succeeded 480a6a1
💚 Build #1578 succeeded 5a0cf94

cc @devcorpio

elasticmachine · 2024-03-04T09:18:04Z

💚 Build Succeeded

Buildkite Build
Commit: 0b8a902

History

💚 Build #1692 succeeded 70a7d46
💚 Build #1606 succeeded 257381b
💚 Build #1593 succeeded c0e559c
💚 Build #1592 succeeded 3d0aba5
💚 Build #1591 succeeded 480a6a1
💚 Build #1570 succeeded 5a0cf94

cc @devcorpio

elasticmachine · 2024-03-04T09:18:05Z

💚 Build Succeeded

Buildkite Build
Commit: 0b8a902

History

💚 Build #856 succeeded 70a7d46
💚 Build #770 succeeded 257381b
💚 Build #757 succeeded c0e559c
💚 Build #756 succeeded 3d0aba5
💚 Build #755 succeeded 480a6a1
💚 Build #734 succeeded 5a0cf94

cc @devcorpio

elasticmachine · 2024-03-04T09:56:08Z

💚 Build Succeeded

Buildkite Build
Commit: 0b8a902

History

💔 Build #3328 failed 70a7d46
💔 Build #3242 failed 257381b
💚 Build #3229 succeeded c0e559c
💔 Build #3227 failed 480a6a1
💔 Build #3206 failed 5a0cf94

cc @devcorpio

elasticmachine · 2024-03-04T09:59:58Z

💚 Build Succeeded

Buildkite Build
Commit: 0b8a902

History

💚 Build #1989 succeeded 70a7d46
💚 Build #1903 succeeded 257381b
💚 Build #1890 succeeded c0e559c
💚 Build #1867 succeeded 5a0cf94
💚 Build #1844 succeeded 627bde3
💔 Build #1611 failed aa29aa8

cc @devcorpio

… not for 4xx (#37981) * only retry when the status is 5xx * remove test AAA comments * add changelog * correct changelog modification * fix ES query * change error handling strategy * do not retry when there is malformed data * improve retry mechanism * improve log message * improve changelog * fix log format (cherry picked from commit 27cde87)

… not for 4xx (#37981) (#38163) * only retry when the status is 5xx * remove test AAA comments * add changelog * correct changelog modification * fix ES query * change error handling strategy * do not retry when there is malformed data * improve retry mechanism * improve log message * improve changelog * fix log format (cherry picked from commit 27cde87) Co-authored-by: Alberto Delgado Roda <[email protected]>

devcorpio added bug Team:obs-ds-hosted-services Label for the Observability Hosted Services team backport-skip Skip notification from the automated backport with mergify labels Feb 12, 2024

devcorpio requested review from vigneshshanmugam and emilioalvap February 12, 2024 15:31

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 Feb 12, 2024

devcorpio commented Feb 12, 2024

View reviewed changes

devcorpio marked this pull request as ready for review February 12, 2024 15:39

devcorpio requested a review from a team as a code owner February 12, 2024 15:39

devcorpio commented Feb 12, 2024

View reviewed changes

devcorpio added backport-v8.12.0 Automated backport with mergify and removed backport-skip Skip notification from the automated backport with mergify labels Feb 12, 2024

emilioalvap reviewed Feb 12, 2024

View reviewed changes

CHANGELOG.next.asciidoc Outdated Show resolved Hide resolved

mergify bot assigned devcorpio Feb 12, 2024

devcorpio force-pushed the state-loader-retry branch 2 times, most recently from e479310 to aa29aa8 Compare February 12, 2024 17:23

vigneshshanmugam requested changes Feb 12, 2024

View reviewed changes

devcorpio requested a review from vigneshshanmugam February 22, 2024 17:37

devcorpio force-pushed the state-loader-retry branch from 480a6a1 to 3d0aba5 Compare February 22, 2024 17:40

devcorpio requested a review from emilioalvap February 22, 2024 17:41

devcorpio removed the backport-v8.12.0 Automated backport with mergify label Feb 22, 2024

vigneshshanmugam reviewed Feb 22, 2024

View reviewed changes

devcorpio added the backport-v8.13.0 Automated backport with mergify label Feb 23, 2024

devcorpio requested a review from vigneshshanmugam February 23, 2024 17:12

vigneshshanmugam approved these changes Feb 23, 2024

View reviewed changes

devcorpio added 11 commits March 4, 2024 10:14

only retry when the status is 5xx

bf4ee72

remove test AAA comments

568be03

add changelog

aa2c02b

correct changelog modification

7c074d2

fix ES query

dd757c0

change error handling strategy

b3e419d

do not retry when there is malformed data

ff6f4cd

improve retry mechanism

c407305

improve log message

639cb20

improve changelog

d305b86

fix log format

0b8a902

devcorpio force-pushed the state-loader-retry branch from 70a7d46 to 0b8a902 Compare March 4, 2024 09:15

devcorpio merged commit 27cde87 into elastic:main Mar 4, 2024
42 of 44 checks passed

devcorpio deleted the state-loader-retry branch March 4, 2024 10:09

mergify bot mentioned this pull request Mar 4, 2024

[8.13](backport #37981) [Heartbeat] Adjust State loader to only retry for failed requests and not for 4xx #38163

Merged

anuj-elastic unassigned devcorpio Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Heartbeat] Adjust State loader to only retry for failed requests and not for 4xx #37981

[Heartbeat] Adjust State loader to only retry for failed requests and not for 4xx #37981

devcorpio commented Feb 12, 2024

devcorpio Feb 12, 2024 •

edited

Loading

vigneshshanmugam Feb 12, 2024

devcorpio Feb 12, 2024 •

edited

Loading

elasticmachine commented Feb 12, 2024

elasticmachine commented Feb 12, 2024 •

edited

Loading

Build stats

devcorpio Feb 12, 2024 •

edited

Loading

vigneshshanmugam Feb 12, 2024

vigneshshanmugam left a comment •

edited

Loading

vigneshshanmugam Feb 12, 2024

vigneshshanmugam Feb 12, 2024

devcorpio Feb 21, 2024 •

edited

Loading

devcorpio Feb 22, 2024

vigneshshanmugam Feb 12, 2024

vigneshshanmugam Feb 12, 2024

devcorpio commented Feb 22, 2024

mergify bot commented Feb 22, 2024

vigneshshanmugam left a comment

devcorpio commented Feb 23, 2024

vigneshshanmugam left a comment

vigneshshanmugam Feb 23, 2024

elasticmachine commented Mar 4, 2024

elasticmachine commented Mar 4, 2024

elasticmachine commented Mar 4, 2024

elasticmachine commented Mar 4, 2024

elasticmachine commented Mar 4, 2024

elasticmachine commented Mar 4, 2024

[Heartbeat] Adjust State loader to only retry for failed requests and not for 4xx #37981

[Heartbeat] Adjust State loader to only retry for failed requests and not for 4xx #37981

Conversation

devcorpio commented Feb 12, 2024

Proposed commit message

Additional context

Checklist

devcorpio Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devcorpio Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Feb 12, 2024

elasticmachine commented Feb 12, 2024 • edited Loading

💚 Build Succeeded

Build stats

❕ Flaky test report

🤖 GitHub comments

devcorpio Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vigneshshanmugam left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devcorpio Feb 21, 2024 • edited Loading

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

devcorpio commented Feb 22, 2024

mergify bot commented Feb 22, 2024

vigneshshanmugam left a comment

Choose a reason for hiding this comment

devcorpio commented Feb 23, 2024

vigneshshanmugam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Mar 4, 2024

💚 Build Succeeded

History

elasticmachine commented Mar 4, 2024

💚 Build Succeeded

History

elasticmachine commented Mar 4, 2024

💚 Build Succeeded

History

elasticmachine commented Mar 4, 2024

💚 Build Succeeded

History

elasticmachine commented Mar 4, 2024

💚 Build Succeeded

History

elasticmachine commented Mar 4, 2024

💚 Build Succeeded

History

devcorpio Feb 12, 2024 •

edited

Loading

devcorpio Feb 12, 2024 •

edited

Loading

elasticmachine commented Feb 12, 2024 •

edited

Loading

devcorpio Feb 12, 2024 •

edited

Loading

vigneshshanmugam left a comment •

edited

Loading

devcorpio Feb 21, 2024 •

edited

Loading