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

Convert heartbeat to new publisher pipeline #4591

Merged
merged 3 commits into from
Jul 3, 2017

Conversation

urso
Copy link

@urso urso commented Jul 2, 2017

Requires: #4554

  • install EventMetadata processing with beat.Client on connect
  • now Task and Job have different return type:
    • a Task can optionally return event fields of type common.MapStr
    • a Job returns/reports full events by wrapping the tasks events into a beat.Event type with additional annotations and start timestamp
    • unexport some 'local' types
  • slight enhancements to fields/tags testing in heartbeat system tests
  • add 'processors' setting to heartbeat monitors (requires doc update)

@urso urso added in progress Pull request is currently in progress. needs_docs labels Jul 2, 2017
@urso urso force-pushed the pipeline/heartbeat branch from ea13047 to 5ed4fd4 Compare July 3, 2017 05:57
urso added 2 commits July 3, 2017 10:05
- install EventMetadata processing with beat.Client on connect
- split in return types between Task and Job:
  - a Task can optionally return event fields of type common.MapStr
  - a Job returns/reports full events by wrapping the Tasks return into a
    beat.Event type with additional annotations
- unexport some 'local' types
@urso urso force-pushed the pipeline/heartbeat branch from 5ed4fd4 to 4724c76 Compare July 3, 2017 08:05
@urso urso added review and removed in progress Pull request is currently in progress. labels Jul 3, 2017
@@ -66,11 +62,12 @@ func (bt *Heartbeat) Run(b *beat.Beat) error {

<-bt.done

bt.manager.Stop()
Copy link
Member

Choose a reason for hiding this comment

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

could also be done as a defer statement on line 46 I think? Is there a reason you use bt.manager instead of manager`

Copy link
Author

Choose a reason for hiding this comment

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

line 46 is another function

t.cancel = m.manager.jobControl.Add(t.config.Schedule, id, job)
processors, err := processors.New(t.config.Processors)
if err != nil {
logp.Critical("Fail to load monitor processors: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

Not for this PR but thinking what we should do in this case? Alternatively we could return an error and move the creating of the jobs into a second for loop. Means all jobs only get created if all tasks work. Not sure what the user expects.

Copy link
Author

Choose a reason for hiding this comment

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

This loop is about creating tasks, after merging the tasks settings (via json file) with the actual monitor settings. That is, this loop also drives monitors config reloading and is to be replaced is metricbeat/filebeat config reloading. In the filebeat/metricbeat case, the reloading is less flexible in handline settings, allowing us to load the configs once and not per task.

@urso urso force-pushed the pipeline/heartbeat branch from 4724c76 to bc9c4c9 Compare July 3, 2017 08:25
func (f funcTask) Run() (common.MapStr, []TaskRunner, error) { return f.run() }

/*
Copy link
Member

Choose a reason for hiding this comment

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

Leftover?

@ruflin ruflin merged commit 6d8a87a into elastic:master Jul 3, 2017
@urso urso mentioned this pull request Jul 3, 2017
22 tasks
@dedemorton dedemorton mentioned this pull request Jul 25, 2017
42 tasks
@dedemorton dedemorton mentioned this pull request Dec 14, 2017
37 tasks
@dedemorton
Copy link
Contributor

dedemorton commented Jan 22, 2018

@urso I'm removing the needs_docs label here because you've already added the processors docs. If additional doc work is required, please open an issue against the docs.

@urso urso deleted the pipeline/heartbeat branch February 19, 2019 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants