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.

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

[heartbeat] Only add monitor.status to browser summary events #29460

Merged
merged 3 commits into from
Dec 17, 2021

Conversation

andrewvc
Copy link
Contributor

@andrewvc andrewvc commented Dec 16, 2021

Fixes #26325

we now only add monitor status to summary events for browsers. Additionally, this renames the poorly renamed wrappers/monitor* to wrappers/wrappers*

Why is it important?

monitor.status should really only be set on unique components of events that constitute a full check. For lightweight checks that means checking an individual IP etc. A step represents progress toward a check, not the check itself. Hence this change.

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

We'll want to check that this doesn't break any features in the synthetics UI as well. I'd index to kibana and check that nothing breaks.

@andrewvc andrewvc added bug Heartbeat Team:obs-ds-hosted-services Label for the Observability Hosted Services team v8.0.0 backport-v8.0.0 Automated backport with mergify labels Dec 16, 2021
@andrewvc andrewvc self-assigned this Dec 16, 2021
@andrewvc andrewvc requested a review from a team as a code owner December 16, 2021 02:53
@elasticmachine
Copy link
Collaborator

Pinging @elastic/uptime (Team:Uptime)

@botelastic 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 Dec 16, 2021
@andrewvc andrewvc changed the title Only add monitor.status to browser summary events [heartbeat] Only add monitor.status to browser summary events Dec 16, 2021
@andrewvc andrewvc marked this pull request as draft December 16, 2021 03:04
@andrewvc andrewvc marked this pull request as ready for review December 16, 2021 03:18
@elasticmachine
Copy link
Collaborator

elasticmachine commented Dec 16, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-12-17T22:10:37.879+0000

  • Duration: 73 min 30 sec

  • Commit: 1bbc479

Test stats 🧪

Test Results
Failed 0
Passed 3277
Skipped 71
Total 3348

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

Copy link
Contributor

@justinkambic justinkambic left a comment

Choose a reason for hiding this comment

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

Code LGTM and I tested this against master and the documents I was seeing were in keeping with the design goals.

@andrewvc andrewvc merged commit e3b1183 into elastic:master Dec 17, 2021
@andrewvc andrewvc deleted the stop-adding-step-status branch December 17, 2021 23:33
mergify bot pushed a commit that referenced this pull request Dec 17, 2021
we now only add monitor status to summary events for browsers. Additionally, this renames the poorly renamed wrappers/monitor* to wrappers/wrappers*

(cherry picked from commit e3b1183)
@andrewvc
Copy link
Contributor Author

@Mergifyio backport 7.17

mergify bot pushed a commit that referenced this pull request Dec 17, 2021
we now only add monitor status to summary events for browsers. Additionally, this renames the poorly renamed wrappers/monitor* to wrappers/wrappers*

(cherry picked from commit e3b1183)
@mergify
Copy link
Contributor

mergify bot commented Dec 17, 2021

backport 7.17

✅ Backports have been created

vigneshshanmugam pushed a commit that referenced this pull request Dec 18, 2021
#29510)

we now only add monitor status to summary events for browsers. Additionally, this renames the poorly renamed wrappers/monitor* to wrappers/wrappers*

(cherry picked from commit e3b1183)

Co-authored-by: Andrew Cholakian <[email protected]>
vigneshshanmugam pushed a commit that referenced this pull request Dec 18, 2021
#29509)

we now only add monitor status to summary events for browsers. Additionally, this renames the poorly renamed wrappers/monitor* to wrappers/wrappers*

(cherry picked from commit e3b1183)

Co-authored-by: Andrew Cholakian <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v8.0.0 Automated backport with mergify bug Heartbeat Team:obs-ds-hosted-services Label for the Observability Hosted Services team v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Heartbeat] Stopping adding monitor.status field to step/end events
4 participants