Unify heartbeat jobs and tasks for simplicity #9258

andrewvc · 2018-11-27T22:20:09Z

In heartbeat we have separate notions of jobs and tasks.

One thing that's hard to do today is to invoke one monitor from another due to the distinction. A monitor has a top-level job that spawns many tasks. For a task to spawn a job without this patch would require an awkward and confusing unwrap / rewrap. This paves the way for enhanced multi-part jobs like #9122 . The other thing this does is move the hoisting of errors from Job return values to fields up into the highest level creation functions. This was required for the composition of jobs as in #9122. It's also cleaner, since we use the error return value for errors up until we no longer can.

Just as important as the composition benefits are the code readibility benefits. This patch makes the code easier to follow. By removing these different notions of Job and Task navigating the execution of a monitor is made simpler since there are fewer types to reason about.

There is probably a tiny amount of extra execution overhead. Tasks only passed a single common.MapStr, where we now pass a *beat.Event, however, the benefit is that we have a more consistent and simple codebase.

heartbeat/monitors/util.go

libbeat/cmd/instance/imports.go

heartbeat/monitors/job.go

heartbeat/monitors/active/http/task.go

heartbeat/monitors/active/dialchain/builder.go

heartbeat/monitors/active/http/task.go

heartbeat/monitors/active/http/http.go

andrewvc · 2018-11-27T23:24:55Z

heartbeat/monitors/job.go

+func AfterJob(j Job, after func(*beat.Event, []Job, error) (*beat.Event, []Job, error)) Job {
+
+	return CreateNamedJob(
+		j.Name(),


Should we just copy the name, or proxy the execution here? I'm thinking there's a good argument for either one, but the copy is a little simpler. These names should be pretty stable.

Not sure I understand the other option you propose.

The other option is to have name be a function rather than a field. Every time the name was asked for we'd invoke the child job. That would allow names to be more dynamic and lazy.

OTOH, I don't know if we actually need that. This is def simpler.

andrewvc · 2018-11-27T23:28:23Z

heartbeat/monitors/util.go

+	return CreateNamedJob(
+		job.Name(),
+		func() (*beat.Event, []Job, error) {
+			start := time.Now()


IIRC the original behavior may have been incorrect, starting the timer when the Job was created, not when it was invoked. This behavior, at any rate, is def correct.

ruflin

I left a few comments and questions. I think the part I didn't fully follow is the move around of the settings.

ruflin · 2018-11-29T07:46:11Z

heartbeat/monitors/active/dialchain/builder.go

-	jobName := jobName(typ, scheme, endpoint.Host, endpoint.Ports)
-	settings := monitors.MakeHostJobSettings(jobName, endpoint.Host, mode).WithFields(fields)
+	jobID := jobID(typ, scheme, endpoint.Host, endpoint.Ports)
+	settings := monitors.MakeHostJobSettings(jobID, endpoint.Host, mode)


Note for myself: Trying to understand why WithFields was moved from there to line 204.

That's because those fields used to be part of the job settings. A general theme in this patch is applying Job level stuff higher up in the call stack, whereas before those options were passed in to a deep point, then used from annotate. The wrapping strategy now used is more clear IMHO.

heartbeat/monitors/active/dialchain/net.go

heartbeat/monitors/active/dialchain/socks5.go

heartbeat/monitors/active/http/http.go

heartbeat/monitors/active/http/task.go

ruflin · 2018-11-29T07:54:03Z

heartbeat/monitors/job.go

+func AfterJob(j Job, after func(*beat.Event, []Job, error) (*beat.Event, []Job, error)) Job {
+
+	return CreateNamedJob(
+		j.Name(),


Not sure I understand the other option you propose.

ruflin · 2018-11-29T07:55:45Z

heartbeat/monitors/monitor.go

 // will cause it to run with the given scheduler until Stop() is called.
 type Monitor struct {
 	name       string
 	config     *common.Config
 	registrar  *pluginsReg
 	uniqueName string
 	scheduler  *scheduler.Scheduler
-	jobTasks   []*task
+	jobTasks   []*configuredJob


Potentially confusing that we have a variable jobTasks with the task part inside? Not sure i have good alternatives.

Yeah, I agree that's really confusing, that's a holdover. I think we should just call it configuredJobs

or just jobs :)

The only thing I don't like about jobs is that then we can call jobs[0].job. I'm going with configuredJobs for now. Glad to discuss further if you think that isn't a great reason.

ruflin · 2018-11-29T07:58:38Z

heartbeat/monitors/util.go

-					"status":   status,
-				},
+			MergeEventFields(event, common.MapStr{
+				"error": look.Reason(err),


How does the event look like this creates? Just want to make sure error is an object and not a keyword.

It returns an object with two fields type, and message. This should be identical to the existing behavior (and we have tests backing this)

heartbeat/monitors/util.go

ruflin · 2018-11-29T08:01:40Z

heartbeat/monitors/util.go

-	return MakeCont(func() (common.MapStr, []TaskRunner, error) {
-		event, cont, err := r.Run()
-		if event != nil {
-			event = event.Clone()


I think this clone here is important to prevent races but I can't find it in the code anymore in MergeEventFields.

Good catch.

So, I'm not clear if we actually do need this. I could understand us cloning the passed in fields, but cloning the event seems weird. That should not have any shared values.

I don't think we need the clone.

@urso your thoughts on this?

Let's make to double check this. Can you also check when this was introduced and the PR comments for it?

Looks like it was introduced in #6950 , which did this to prevent races among shared structures in the case continuations were created. After a good amount of spelunking I don't see any races.

However, reasoning about those races is really hard to do, you basically need to bootstrap all the code into your head and assume that you understand it correctly.

That said, I don't think the approach of fixing it in this spot is necessarily correct. This assumes that WithFields is the only place mutation happens.

I'd like to think about this for a bit and see if there's a more bullet-proof solution I can think of (short of moving toward immutable datastructures).

What you see as disadvantages of 1?

To be clear, I don't think 1 and 2 are competing proposals. These are separate issues with separate solutions. I don't see any disadvantages with that strategy aside from it always being a bit awkward for functions to mutate their arguments.

What is your suggestion to move forward and not reintroduce the potential race?

My suggestion is to re-introduce the clone in this PR and then introduce a subsequent PR that restructures things so as to make it unnecessary. That would basically mean rewriting all the function signatures for everything here. I got pretty far with it on a new branch but I don't think it's worth the effort right at this moment, and I think it'd make this PR far too unwieldy to review.

+1 on your approach. Like this we can also discuss the follow up changes separately.

heartbeat/monitors/task.go

andrewvc · 2018-11-29T21:33:56Z

@urso @ruflin pushed a set of improvements incorporating your feedback. Awaiting y'alls input on a few outstanding things before we can wrap this one up.

elasticmachine · 2018-12-03T15:15:18Z

Pinging @elastic/uptime

heartbeat/monitors/job.go

andrewvc · 2018-12-13T21:03:05Z

@urso @ruflin I wound up discovering that it was simple enough just to finish the refactor of method signatures. All tests are passing.

TBH this PR turned into a monster. It was intended as a supporting PR for the AWS autodiscovery which may not happen. It's still a good improvement in code quality, but I wouldn't have started it if I'd known we wouldn't release that feature. Given that it's here, however, it is worth a merge.

ruflin · 2018-12-14T10:13:40Z

Looks like some updates to the tests are needed.

andrewvc · 2018-12-18T01:51:12Z

OK, looks like there was a small behavior change where we still logged an http.rtt.total field in the case of an I/O error. Fixed with the latest push.

ruflin · 2018-12-18T08:53:58Z

Can you rebase again on master. There was an issue introduce in master which made the build fail which should be fixed now. Like this also Travis should run through.

Overall LGTM.

In heartbeat we have separate notions of jobs and tasks. This can make following the code and sharing the code among monitors confusing. This patch unifies the two. There is a tiny amount of extra execution overhead. Tasks only passed a single `common.MapStr`, where we now pass a `*beat.Event`, however, the benefit is that we have a more consistent and simple codebase.

This lets us reason about concurrency more easily since jobs are now explicitly about mutation. This should also, theoretically, cut down on garbage. Practically we still use a lot of map merging however.

andrewvc · 2018-12-18T20:59:48Z

Woohoo! Looks like, with the exception of the broken mac workers, CI is happy!

Unify heartbeat jobs and tasks for simplicity In heartbeat we have separate notions of jobs and tasks. This can make following the code and sharing the code among monitors confusing. This patch unifies the two. There is a tiny amount of extra execution overhead. Tasks only passed a single `common.MapStr`, where we now pass a `*beat.Event`, however, the benefit is that we have a more consistent and simple codebase. Also makes events arguments to Jobs rather than return values. This lets us reason about concurrency more easily since jobs are now explicitly about mutation, and accidentally sharing data between tasks is made more difficult. (cherry picked from commit 37a5b1f)

andrewvc added in progress Pull request is currently in progress. Heartbeat Team:obs-ds-hosted-services Label for the Observability Hosted Services team labels Nov 27, 2018

houndci-bot reviewed Nov 27, 2018

View reviewed changes

andrewvc force-pushed the unify-jobs branch from 13a42b8 to fb70739 Compare November 27, 2018 22:27

houndci-bot reviewed Nov 27, 2018

View reviewed changes

andrewvc force-pushed the unify-jobs branch from 4c8e395 to bf4a124 Compare November 27, 2018 22:58

andrewvc added enhancement refactoring and removed enhancement labels Nov 27, 2018

andrewvc requested review from urso and ruflin November 27, 2018 23:09

andrewvc force-pushed the unify-jobs branch from bf4a124 to 1ebbb81 Compare November 27, 2018 23:12

andrewvc commented Nov 27, 2018

View reviewed changes

andrewvc added review and removed in progress Pull request is currently in progress. labels Nov 27, 2018

ruflin reviewed Nov 29, 2018

View reviewed changes

urso reviewed Nov 29, 2018

View reviewed changes

heartbeat/monitors/task.go Outdated Show resolved Hide resolved

andrewvc force-pushed the unify-jobs branch from 80cbb66 to c09ac38 Compare November 29, 2018 21:30

andrewvc closed this Nov 29, 2018

andrewvc reopened this Nov 29, 2018

alvarolobato assigned andrewvc Dec 3, 2018

ruflin added Team:obs-ds-hosted-services Label for the Observability Hosted Services team and removed Team:obs-ds-hosted-services Label for the Observability Hosted Services team labels Dec 3, 2018

houndci-bot reviewed Dec 13, 2018

View reviewed changes

heartbeat/monitors/job.go Outdated Show resolved Hide resolved

heartbeat/monitors/job.go Outdated Show resolved Hide resolved

heartbeat/monitors/job.go Outdated Show resolved Hide resolved

heartbeat/monitors/job.go Outdated Show resolved Hide resolved

andrewvc force-pushed the unify-jobs branch from d62c638 to f9f704b Compare December 13, 2018 20:09

andrewvc force-pushed the unify-jobs branch from f9f704b to 723a8a3 Compare December 13, 2018 21:00

andrewvc force-pushed the unify-jobs branch from 723a8a3 to 426adc3 Compare December 13, 2018 21:04

andrewvc force-pushed the unify-jobs branch from c3d5c41 to 9e1104c Compare December 15, 2018 00:19

andrewvc added 7 commits December 18, 2018 11:44

Incorporate hound feedback + tweaks

54a7151

More cleanup

4d54514

Incorporate PR feedback

8f827b4

Make events arguments to Jobs rather than return values.

a030eae

This lets us reason about concurrency more easily since jobs are now explicitly about mutation. This should also, theoretically, cut down on garbage. Practically we still use a lot of map merging however.

revert heartbeat.yml to master

8719ffd

Match earlier behavior around HTTP rtts when an IO error occurs

374444a

andrewvc force-pushed the unify-jobs branch from 2bfeb31 to 374444a Compare December 18, 2018 17:45

andrewvc merged commit 37a5b1f into elastic:master Dec 19, 2018

andrewvc mentioned this pull request Dec 19, 2018

Cherry-pick #8984 to 6.x: Add heartbeat test for TLS client cert auth #9676

Merged

andrewvc mentioned this pull request Dec 21, 2018

Cherry-pick #9258 to 6.x: Unify heartbeat jobs and tasks for simplicity #9736

Merged

andrewvc added the v6.7.0 label Dec 21, 2018

andrewvc mentioned this pull request Dec 22, 2018

[heartbeat] Set IDs explicitly #9697

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify heartbeat jobs and tasks for simplicity #9258

Unify heartbeat jobs and tasks for simplicity #9258

andrewvc commented Nov 27, 2018 •

edited by alvarolobato

Loading

andrewvc Nov 27, 2018

ruflin Nov 29, 2018

andrewvc Nov 29, 2018

andrewvc Nov 29, 2018

andrewvc Nov 27, 2018

ruflin left a comment

ruflin Nov 29, 2018

andrewvc Nov 29, 2018

ruflin Nov 29, 2018

ruflin Nov 29, 2018

andrewvc Nov 29, 2018

urso Nov 29, 2018

andrewvc Nov 29, 2018

ruflin Nov 29, 2018

andrewvc Nov 29, 2018

ruflin Nov 29, 2018

andrewvc Nov 29, 2018

andrewvc Nov 29, 2018

ruflin Dec 3, 2018

andrewvc Dec 6, 2018

ruflin Dec 7, 2018

andrewvc Dec 7, 2018

ruflin Dec 10, 2018

andrewvc Dec 13, 2018

ruflin Dec 13, 2018

andrewvc commented Nov 29, 2018

elasticmachine commented Dec 3, 2018

andrewvc commented Dec 13, 2018

ruflin commented Dec 14, 2018

andrewvc commented Dec 18, 2018

ruflin commented Dec 18, 2018

andrewvc commented Dec 18, 2018

Unify heartbeat jobs and tasks for simplicity #9258

Unify heartbeat jobs and tasks for simplicity #9258

Conversation

andrewvc commented Nov 27, 2018 • edited by alvarolobato 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

Choose a reason for hiding this comment

ruflin 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

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

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 Nov 29, 2018

elasticmachine commented Dec 3, 2018

andrewvc commented Dec 13, 2018

ruflin commented Dec 14, 2018

andrewvc commented Dec 18, 2018

ruflin commented Dec 18, 2018

andrewvc commented Dec 18, 2018

andrewvc commented Nov 27, 2018 •

edited by alvarolobato

Loading