[Heartbeat] Refactor config system #23467

andrewvc · 2021-01-12T19:51:06Z

This PR:

Refactors the config for synthetics suites to act as regular monitors under heartbeat.monitors rather than have the new top-level synthetic.suites syntax we've been using so far.
Changes the behavior of local suites to copy their data into the container rather than run directly off the shared docker volume
Adds a first-class notion of aliasing to monitor types, so that synthetics/http monitors show up as http, and not as a distinct type (applies to all monitor types)
Simplifies the types associated with monitor plugins into a new plugin.Plugin{} struct, rather than passing around multiple values everywhere.
See https://github.com/elastic/beats/pull/23467/files#diff-7f1e5387c4757cca1e98483a5678e377a28ca5f9d77b267a4121a14249c96b82R7 for an example of the new config syntax

More on the change to copying local tests into the container

The rationale here is that doing so resolves any file permissions issues that may be present due to the suite directory being shared to the container as read only OR due to incompatible UIDs between the docker container and the host.

Fixes elastic/synthetics#156
Fixes #23823

As a note, no tests are added here due to the complexity of testing this small amount of I/O functionality, however, any issues should be caught by our E2E tests in https://github.com/elastic/synthetics/tree/master/__tests__/e2e . I've opened an issue to improve this situation here: #23346

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.

Author's Checklist

[ ]

How to test this PR locally

Related issues

Takes over from #23347
Fixes elastic/synthetics#156
Fixes elastic/uptime#271

Use cases

Screenshots

Logs

The rationale here is that doing so resolves any file permissions issues that may be present due to the suite directory being shared to the container as read only OR due to incompatible UIDs between the docker container and the host. Fixes elastic/synthetics#156

andrewvc · 2021-01-14T22:21:46Z

heartbeat/monitors/wrappers/monitors.go

@@ -220,15 +234,7 @@ func makeAddSummary(monitorType string) jobs.JobWrapper {
 			if state.remaining == 0 {
 				up := state.up
 				down := state.down
-				if monitorType == "browser" {


Now handled directly in the browser type

andrewvc · 2021-02-02T21:04:52Z

x-pack/heartbeat/monitors/browser/source/local.go

+
+// setupOnlineDir is run in environments with internet access and attempts to make sure the node env
+// is setup correctly.
+func setupOnlineDir(dir string) (err error) {


I don't have tests for this because this requires an internet connection and NPM to be functioning. Our E2E tests in synthetics will ultimately catch issues here. See #23346

elasticmachine · 2021-02-02T22:25:33Z

Pinging @elastic/uptime (Team:Uptime)

blakerouse

Wow that was big! Overall this looks good.

I made a few comments, but no blockers. Also seems like your imports are random. I know we have discussed that before (about how annoying go fmt doesnt fix it), but it just make the code look dirty.

blakerouse · 2021-02-11T17:17:20Z

heartbeat/beater/heartbeat.go

@@ -131,22 +124,30 @@ func (bt *Heartbeat) Run(b *beat.Beat) error {
 }

 // RunStaticMonitors runs the `heartbeat.monitors` portion of the yaml config if present.
-func (bt *Heartbeat) RunStaticMonitors(b *beat.Beat) error {
+func (bt *Heartbeat) RunStaticMonitors(b *beat.Beat) (stop func(), err error) {


I think this can be change to RunMonitors, seems that there is only one type of monitor at this level of the code.

blakerouse · 2021-02-11T17:17:59Z

heartbeat/monitors/active/icmp/icmp.go

@@ -89,29 +90,30 @@ func (jf *jobFactory) checkConfig() error {
 	return nil
 }

-func (jf *jobFactory) makeJobs() (j []jobs.Job, endpoints int, err error) {
+func (jf *jobFactory) makePlugin() (plugin2 plugin.Plugin, err error) {


Why the names returned parameters? Seems that the code always defines the return in the statement of the function.

blakerouse · 2021-02-11T17:21:00Z

heartbeat/monitors/active/icmp/icmp.go

 		}

 		j = append(j, wrappers.WithURLField(u, job))
 	}

-	return j, len(jf.config.Hosts), nil
+	return plugin.Plugin{Jobs: j, Close: nil, Endpoints: len(jf.config.Hosts)}, nil


Instead of having the caller know if Close is set, it would be better to always set it (to an empty function).

Or add a Close() function directly on the plugin that does the checking and calling of the actual function.

Will do in a follow-up!

heartbeat/monitors/mocks_test.go

blakerouse · 2021-02-11T17:23:55Z

x-pack/heartbeat/monitors/browser/browser.go

 	"github.com/elastic/beats/v7/libbeat/common"
 	"github.com/elastic/beats/v7/libbeat/logp"
 	"github.com/elastic/beats/v7/x-pack/heartbeat/monitors/browser/synthexec"
 )

 func init() {
-	monitors.RegisterActive("browser", create)
-	monitors.RegisterActive("synthetic/browser", create)
+	plugin.Register("browser", create, "synthetic", "synthetics/synthetic")


Why the rename form synthetics/browser to just synthetics/synthetic? That just seems like weird naming.

In the future we'd like to have a synthetics/api, and we use the browser language across the board from UI to backend

* Refactors the config for synthetics suites to act as regular monitors under `heartbeat.monitors` rather than have the new top-level `synthetic.suites` syntax we've been using so far. * Changes the behavior of local suites to copy their data into the container rather than run directly off the shared docker volume * Adds a first-class notion of aliasing to monitor types, so that `synthetics/http` monitors show up as `http`, and not as a distinct type (applies to all monitor types) * Simplifies the types associated with monitor plugins into a new `plugin.Plugin{}` struct, rather than passing around multiple values everywhere. * See https://github.com/elastic/beats/pull/23467/files#diff-7f1e5387c4757cca1e98483a5678e377a28ca5f9d77b267a4121a14249c96b82R7 for an example of the new config syntax #### More on the change to copying local tests into the container The rationale here is that doing so resolves any file permissions issues that may be present due to the suite directory being shared to the container as read only OR due to incompatible UIDs between the docker container and the host. Fixes elastic/synthetics#156 Fixes elastic#23823 As a note, no tests are added here due to the complexity of testing this small amount of I/O functionality, however, any issues should be caught by our E2E tests in https://github.com/elastic/synthetics/tree/master/__tests__/e2e . I've opened an issue to improve this situation here: elastic#23346 (cherry picked from commit fb25ded)

…dows-7 * upstream/master: (332 commits) Use ECS v1.8.0 (elastic#24086) Add support for postgresql csv logs (elastic#23334) [Heartbeat] Refactor config system (elastic#23467) [CI] install docker-compose with retry (elastic#24069) Add nodes to filebeat-kubernetes.yaml ClusterRole - fixes elastic#24051 (elastic#24052) updating manifest files for filebeat threatintel module (elastic#24074) Add Zeek Signatures (elastic#23772) Update Beats to ECS 1.8.0 (elastic#23465) Support running Docker logging plugin on ARM64 (elastic#24034) Fix ec2 metricset fields.yml and add integration test (elastic#23726) Only build targz and zip versions of Beats if PACKAGES is set in agent (elastic#24060) [Filebeat] Add field definitions for known Netflow/IPFIX vendor fields (elastic#23773) [Elastic Agent] Enroll with Fleet Server (elastic#23865) [Filebeat] Convert logstash logEvent.action objects to strings (elastic#23944) [Ingest Management] Fix reloading of log level for services (elastic#24055) Add Agent standalone k8s manifest (elastic#23679) [Metricbeat][Kubernetes] Extend state_node with more conditions (elastic#23905) [CI] googleStorageUploadExt step (elastic#24048) Check fields are documented for aws metricsets (elastic#23887) Update go-concert to 0.1.0 (elastic#23770) ...

* Refactors the config for synthetics suites to act as regular monitors under `heartbeat.monitors` rather than have the new top-level `synthetic.suites` syntax we've been using so far. * Changes the behavior of local suites to copy their data into the container rather than run directly off the shared docker volume * Adds a first-class notion of aliasing to monitor types, so that `synthetics/http` monitors show up as `http`, and not as a distinct type (applies to all monitor types) * Simplifies the types associated with monitor plugins into a new `plugin.Plugin{}` struct, rather than passing around multiple values everywhere. * See https://github.com/elastic/beats/pull/23467/files#diff-7f1e5387c4757cca1e98483a5678e377a28ca5f9d77b267a4121a14249c96b82R7 for an example of the new config syntax #### More on the change to copying local tests into the container The rationale here is that doing so resolves any file permissions issues that may be present due to the suite directory being shared to the container as read only OR due to incompatible UIDs between the docker container and the host. Fixes elastic/synthetics#156 Fixes #23823 As a note, no tests are added here due to the complexity of testing this small amount of I/O functionality, however, any issues should be caught by our E2E tests in https://github.com/elastic/synthetics/tree/master/__tests__/e2e . I've opened an issue to improve this situation here: #23346 (cherry picked from commit fb25ded)

andrewvc added 13 commits January 4, 2021 13:47

Add changelog

2c48c55

Fix var naming

1ab0fd6

Remote journeyps

168c6a0

Checkpoint

e94fe34

Checkpoint

a6581ba

Checkpoint

3ba5203

Checkpoint

d5aacfe

Suites as jobs

3e47422

Checkpoint

f7971f4

Checkpoint

fd4d03d

Sort of works

d27f695

Reorg

47320ce

andrewvc added enhancement Heartbeat Team:obs-ds-hosted-services Label for the Observability Hosted Services team v7.12.0 labels Jan 12, 2021

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 12, 2021

Simplification attempt

7c05aa6

andrewvc mentioned this pull request Jan 12, 2021

[Uptime] Copy synthetics suites to tmpdir before running #23347

Closed

6 tasks

checkpoint

367548c

andrewvc requested a review from drewr January 13, 2021 22:11

andrewvc added 3 commits January 13, 2021 16:34

Things work again

70f5281

Switch from multiple plugin registration to aliases

3c66ed1

Cleanup

a96277f

andrewvc commented Jan 14, 2021

View reviewed changes

andrewvc added 3 commits January 14, 2021 18:04

Suites almost working, inline works

44628b0

Checkpoint

75250bb

npm i + beginnings of cleanup

54eff79

andrewvc mentioned this pull request Jan 25, 2021

Elastic Agent and Heartbeat Integration - Phase 1 (HTTP/ICMP/TCP Ping) elastic/uptime#235

Closed

10 tasks

andrewvc added 9 commits January 29, 2021 16:06

Add tests for new wrapper behavior

8081b1c

Additional tests

2332b13

Add basic validations for local source

fde5765

Add tests for local source

c82c155

Minimize fixtures

24ced05

Merge remote-tracking branch 'origin/master' into remote-journeys

95d695d

Update go.sum

ebcfb03

Fix linter errors

34469bf

Fix local source behavior to remove node_modules

8ed3ad1

andrewvc commented Feb 2, 2021

View reviewed changes

Improve test coverage

4002755

andrewvc self-assigned this Feb 2, 2021

andrewvc marked this pull request as ready for review February 2, 2021 22:25

andrewvc requested a review from a team as a code owner February 2, 2021 22:25

Add changelog

b7c8d19

andrewvc changed the title ~~Refactor config system~~ [Heartbeat] Refactor config system Feb 2, 2021

andrewvc mentioned this pull request Feb 2, 2021

[Heartbeat] Enhanced Synthetics Config Syntax #23823

Closed

andrewvc added 2 commits February 3, 2021 08:29

Update changelog

6e1a49b

Merge remote-tracking branch 'origin/master' into remote-journeys

3c47a5c

blakerouse approved these changes Feb 11, 2021

View reviewed changes

andrewvc added 2 commits February 17, 2021 07:44

Merge remote-tracking branch 'origin/master' into remote-journeys

4d8c07b

Incorporate PR feedback

8248900

andrewvc merged commit fb25ded into elastic:master Feb 17, 2021

andrewvc deleted the remote-journeys branch February 17, 2021 14:25

andrewvc mentioned this pull request Feb 17, 2021

Cherry-pick #23467 to 7.x: [Heartbeat] Refactor config system #24087

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Heartbeat] Refactor config system #23467

[Heartbeat] Refactor config system #23467

andrewvc commented Jan 12, 2021 •

edited

Loading

andrewvc Jan 14, 2021

andrewvc Feb 2, 2021

elasticmachine commented Feb 2, 2021

blakerouse left a comment

blakerouse Feb 11, 2021

blakerouse Feb 11, 2021

blakerouse Feb 11, 2021

andrewvc Feb 17, 2021

blakerouse Feb 11, 2021

andrewvc Feb 17, 2021

[Heartbeat] Refactor config system #23467

[Heartbeat] Refactor config system #23467

Conversation

andrewvc commented Jan 12, 2021 • edited Loading

More on the change to copying local tests into the container

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Feb 2, 2021

blakerouse left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewvc commented Jan 12, 2021 •

edited

Loading