Marshal heartbeat duration as int64,string to prevent int64 -> float conversion #34280

emilioalvap · 2023-01-16T15:34:37Z

What does this PR do?

Changes marshalling of states duration to string-encoded int64, per recommendation. Fixes #34218.

Why is it important?

When duration_ms gets long enough, it starts being marshalled in float format, which is incompatible with int64 type. It then prevents states from being recovered from ES, generating a new one every ~20min.

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

mergify · 2023-01-16T16:56:47Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @emilioalvap? 🙏.
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-01-16T19:23:14Z

💚 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-01-26T16:37:34.416+0000
Duration: 44 min 51 sec

Test stats 🧪

Test	Results
Failed	0
Passed	1889
Skipped	25
Total	1914

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

emilioalvap · 2023-01-16T19:42:05Z

heartbeat/monitors/wrappers/monitorstate/monitorstate.go

@@ -60,7 +60,7 @@ type State struct {
 	ID string `json:"id"`
 	// StartedAt is the start time of the state, should be the same for a given state ID
 	StartedAt  time.Time   `json:"started_at"`
-	DurationMs int64       `json:"duration_ms"`
+	DurationMs int64       `json:"duration_ms,string"`


Adding this modifier seems easier than changing the struct to a map representation

elasticmachine · 2023-01-16T19:42:29Z

Pinging @elastic/uptime (Team:Uptime)

andrewvc · 2023-01-18T03:36:07Z

What does ES return as the number gets larger here? A number in scientific notation, or just the full number?

I know we don't test ES itself in our tests here. Have you manually tested this with ES yet? I was certain I fixed and tested this in my last PR, but perhaps I missed something when testing it myself.

emilioalvap · 2023-01-18T14:30:13Z

@andrewvc, I tested with ES and event source is being sent with duration in scientific notation when it gets large enough.
We fixed the indexing part, where ES was rejecting documents.

This is an additional step forward, where the field is indexed correctly, but _source in ES still is stored with scientific notation. So now the error happens when loading previous states from ES, heartbeat can't parse the scientific notation to int64 and ends up generating a new state.

andrewvc

LGTM, pending changelog addition

mergify · 2023-01-26T14:32:43Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b emilio-fix-state-marshal upstream/emilio-fix-state-marshal
git merge upstream/main
git push upstream emilio-fix-state-marshal

…conversion (#34280) * Add test initial state and build tag * Keep int64 precision on marshal * Add changelog (cherry picked from commit 762ad18)

…conversion (#34280) (#34401) * Add test initial state and build tag * Keep int64 precision on marshal * Add changelog (cherry picked from commit 762ad18) Co-authored-by: Emilio Alvarez Piñeiro <95703246+emilioalvap@users.noreply.github.com>

…conversion (#34280) * Add test initial state and build tag * Keep int64 precision on marshal * Add changelog

emilioalvap added bug Team:obs-ds-hosted-services Label for the Observability Hosted Services team v8.6.0 labels Jan 16, 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 Jan 16, 2023

mergify bot assigned emilioalvap Jan 16, 2023

emilioalvap force-pushed the emilio-fix-state-marshal branch from 1b36644 to 8acb07a Compare January 16, 2023 19:20

emilioalvap commented Jan 16, 2023

View reviewed changes

emilioalvap marked this pull request as ready for review January 16, 2023 19:42

emilioalvap requested a review from a team as a code owner January 16, 2023 19:42

emilioalvap requested a review from andrewvc January 16, 2023 19:42

emilioalvap changed the title ~~Marshal heartbeat state as map to prevent int64 -> float conversion~~ Marshal heartbeat duration as int64,string to prevent int64 -> float conversion Jan 16, 2023

andrewvc approved these changes Jan 18, 2023

View reviewed changes

emilioalvap added the backport-v8.6.0 Automated backport with mergify label Jan 26, 2023

emilioalvap added 3 commits January 26, 2023 17:34

Add test initial state and build tag

94e4990

Keep int64 precision on marshal

35633e1

Add changelog

98ab15d

emilioalvap force-pushed the emilio-fix-state-marshal branch from 3704dbc to 98ab15d Compare January 26, 2023 16:37

emilioalvap merged commit 762ad18 into elastic:main Jan 26, 2023

mergify bot mentioned this pull request Jan 26, 2023

[8.6](backport #34280) Marshal heartbeat duration as int64,string to prevent int64 -> float conversion #34401

Merged

emilioalvap added v8.7.0 and removed v8.6.0 labels Feb 10, 2023

chrisberkhout pushed a commit that referenced this pull request Jun 1, 2023

Marshal heartbeat duration as int64,string to prevent int64 -> float …

e280fd1

…conversion (#34280) * Add test initial state and build tag * Keep int64 precision on marshal * Add changelog

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Marshal heartbeat duration as int64,string to prevent int64 -> float conversion #34280

Marshal heartbeat duration as int64,string to prevent int64 -> float conversion #34280

emilioalvap commented Jan 16, 2023 •

edited

Loading

mergify bot commented Jan 16, 2023

elasticmachine commented Jan 16, 2023 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

emilioalvap Jan 16, 2023

elasticmachine commented Jan 16, 2023

andrewvc commented Jan 18, 2023

emilioalvap commented Jan 18, 2023

andrewvc left a comment

mergify bot commented Jan 26, 2023

Marshal heartbeat duration as int64,string to prevent int64 -> float conversion #34280

Marshal heartbeat duration as int64,string to prevent int64 -> float conversion #34280

Conversation

emilioalvap commented Jan 16, 2023 • edited Loading

What does this PR do?

Why is it important?

Checklist

How to test this PR locally

mergify bot commented Jan 16, 2023

elasticmachine commented Jan 16, 2023 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

emilioalvap Jan 16, 2023

Choose a reason for hiding this comment

elasticmachine commented Jan 16, 2023

andrewvc commented Jan 18, 2023

emilioalvap commented Jan 18, 2023

andrewvc left a comment

Choose a reason for hiding this comment

mergify bot commented Jan 26, 2023

emilioalvap commented Jan 16, 2023 •

edited

Loading

elasticmachine commented Jan 16, 2023 •

edited by jenkins-beats-ci bot

Loading