Add Fields and Tag to monitor config by glefloch · Pull Request #4141 · elastic/beats

glefloch · 2017-04-28T12:58:05Z

Add Tags and fields attribute to monitor configuration.

elasticmachine · 2017-04-28T13:02:03Z

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

elasticmachine · 2017-04-28T13:02:13Z

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

ruflin · 2017-04-28T13:25:30Z

@glefloch Haven't checked the code yet, but could you add a changelog entry?

glefloch · 2017-04-28T13:26:36Z

Oh yes for sure, I'm doing it right now

ruflin

I haven't looked into it yet but I was kind of hoping we could add the meta data either in monitors package or even beater package as it is generic for all monitors. The way it is added at the moment it has to be implemented for each monitor. Means if someone adds a new monitor the code also has to be added.

Can you check if there is an option to add the code on a more global level?

ruflin · 2017-05-01T06:41:18Z

heartbeat/heartbeat.full.yml

This must be added to heartbeat/_meta/beat.full.yml and then you have to run make update.

Ok, I'm reverting those modifications.

ruflin · 2017-05-01T06:42:21Z

heartbeat/heartbeat.full.yml

I suggest we add it only to one monitor (first one) as an example and not every single one.

ruflin · 2017-05-01T06:42:28Z

heartbeat/heartbeat.full.yml

See comment above.

ruflin · 2017-05-01T06:44:00Z

heartbeat/monitors/active/icmp/icmp.go

Can you call it err instead of e to be consistent with most of the other code?

Yes I will update the name.

codecov · 2017-05-01T19:44:27Z

Codecov Report

Merging #4141 into master will decrease coverage by 0.04%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #4141      +/-   ##
==========================================
- Coverage   64.64%   64.59%   -0.05%     
==========================================
  Files         490      491       +1     
  Lines       34392    34440      +48     
==========================================
+ Hits        22233    22247      +14     
- Misses      10159    10188      +29     
- Partials     2000     2005       +5

Impacted Files	Coverage Δ
heartbeat/monitors/active/http/config.go	`0% <ø> (ø)`	⬆️
heartbeat/monitors/active/http/task.go	`7.92% <0%> (-0.41%)`	⬇️
metricbeat/module/system/process/process.go	`57.81% <0%> (-4.69%)`	⬇️
metricbeat/mb/module/wrapper.go	`84.88% <0%> (-1.15%)`	⬇️
filebeat/prospector/prospector.go	`86.93% <0%> (ø)`	⬆️
filebeat/prospector/prospector_log.go	`82.31% <0%> (ø)`	⬆️
filebeat/prospector/factory.go	`57.89% <0%> (ø)`	⬆️
filebeat/crawler/crawler.go	`79.72% <0%> (ø)`	⬆️
filebeat/channel/outlet.go	`86.66% <0%> (ø)`	⬆️
filebeat/prospector/prospector_stdin.go
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b01fa9...d9aa64c. Read the comment docs.

glefloch · 2017-05-01T19:48:41Z

@ruflin, I was expecting the same thing before starting working on this issue. But there I didn't found any common configuration object between monitors, that's why I added the metadata object in each monitor configuration.
Have you any ideas on how I could make this more 'common'?

ruflin · 2017-05-03T11:04:17Z

I wast thinking perhaps somewhere around MakeJob? https://github.com/elastic/beats/blob/master/heartbeat/monitors/util.go#L92 Or probably even better around createJobTask? https://github.com/elastic/beats/blob/master/heartbeat/beater/manager.go#L241 The tricky part is how to get the config there. @urso would know more but is offline this week.

BTW: This should also be added to the docs under the common config options: https://github.com/elastic/beats/blob/master/heartbeat/docs/reference/configuration/heartbeat-options.asciidoc

urso · 2017-05-11T11:10:48Z

The common configurations for monitors are currently 'implicit' in code (no special type), as originally I have had only one field to load from: See here

With us adding more fields, this should become it's own type.

All settings are added to MonitorTask.

Not sure, but I think MonitorTask can become a package private type + we can make it act more like the final job factory by moving createJobTask to (*MonitorTask).PrepareSchedulerJob(client publisher.Client) scheduler.TaskFunc. Then, right before PublishEvent we can apply the EventMetaData.

I didn't touch any of this code yet, as we were planning to redo the reloading functionality, to resemble filebeat/metricbeat conf.d support.

glefloch · 2017-05-12T20:32:48Z

@urso Thanks for those explanation. I'm going to do what you said and I will push that up.

elasticmachine · 2017-06-05T14:55:46Z

Can one of the admins verify this patch?

glefloch · 2017-06-05T14:56:30Z

I finally got time to work on this. Thanks @urso, your solution is way muck cleaner.

tsg · 2017-06-06T11:55:42Z

@glefloch For the make check failure, you need to run goimports on the source code. Thanks a lot for contributing!

glefloch · 2017-06-06T13:27:52Z

@tsg thanks, I ran make check and fixed code format.

urso

Code change looks quite clean, but I left you 2 minor comments.

Great you also added an integration test. Much appreciated. But checking travis, the test seem to have failed. Any idea why?

urso · 2017-06-09T13:57:46Z

heartbeat/beater/manager.go

the name never changes. we can move this line before returning the func and execute the check only once instead of per event/run...

You are right, I move it on top of the return

urso · 2017-06-09T14:04:40Z

heartbeat/beater/manager.go

we really want to stop the job by returning here? How 'fatal' is an error when adding fields? e.g. we still want to try to report the original event if this op failed? MergeFields only errors if some field can not be spliced into, because some supposed to be intermediate object is an actual leaf/primitive value.

Normall If len(next) > 0, more followup tasks will be executed to finish the final job. We want to kill the job on 'naming' conflicts in MergeFields?

Well, you right, should we also ignore errors on while add tags?

glefloch · 2017-06-09T20:35:50Z

Thanks for the review @urso, I'm going to fix your comments and check the test as well.

tsg · 2017-06-12T11:01:13Z

@glefloch Travis complains about the latest version. Is that test passing locally for you?

glefloch · 2017-06-12T19:43:39Z

@tsg actually, when I run INTEGRATION_TESTS= TESTING_ENVIRONMENT=latest make -C heartbeat testsuite everything looks good but no python tests are ran.. So I'm not able to fix the tests... I'm looking for a fix on my machine.

glefloch · 2017-06-14T06:37:52Z

@tsg, heartbeat testsuite is ok, but filebeat testsuite got broken because of a timeout with elasticsearch... There is also a weird error with the heartbeat testsuite using TEST_ENVIRONMENT=0 it seems that something did not answer within 10min on testsuite initialization.
Is it possible to restart test?

glefloch · 2017-06-23T08:53:03Z

@tsg, I rebase master and all check are ok. Can you check this PR again?

urso · 2017-06-26T12:13:30Z

Merged. Thanks for you patience and time getting this change in.

glefloch · 2017-06-26T12:19:26Z

You're welcome that was a pleasure to implement this feature.

dedemorton · 2017-08-17T01:19:24Z

Removing need_docs tag. Docs were added in #4753

ruflin added Heartbeat review labels Apr 28, 2017

ruflin requested a review from urso April 28, 2017 13:25

ruflin reviewed May 1, 2017

View reviewed changes

ruflin added the in progress Pull request is currently in progress. label May 4, 2017

glefloch force-pushed the fix/3623 branch from d9aa64c to cd2d311 Compare June 5, 2017 14:55

glefloch force-pushed the fix/3623 branch from cd2d311 to 90feb12 Compare June 5, 2017 14:59

glefloch force-pushed the fix/3623 branch from 90feb12 to 1cf13b2 Compare June 6, 2017 13:26

glefloch force-pushed the fix/3623 branch 2 times, most recently from 5cd9516 to 70e97b6 Compare June 8, 2017 08:32

urso reviewed Jun 9, 2017

View reviewed changes

urso approved these changes Jun 12, 2017

View reviewed changes

glefloch force-pushed the fix/3623 branch from 2b99269 to c5b8941 Compare June 17, 2017 12:12

Add Fields and Tag to monitor config

837d312

glefloch force-pushed the fix/3623 branch from c5b8941 to 837d312 Compare June 23, 2017 07:39

urso added needs_docs and removed in progress Pull request is currently in progress. labels Jun 26, 2017

urso merged commit be38ce2 into elastic:master Jun 26, 2017

glefloch deleted the fix/3623 branch June 26, 2017 12:18

dedemorton mentioned this pull request Jul 25, 2017

Beats doc updates in 6.0 #4540

Closed

42 tasks

dedemorton removed the needs_docs label Aug 17, 2017

Conversation

glefloch commented Apr 28, 2017

Uh oh!

elasticmachine commented Apr 28, 2017

Uh oh!

elasticmachine commented Apr 28, 2017

Uh oh!

ruflin commented Apr 28, 2017

Uh oh!

glefloch commented Apr 28, 2017

Uh oh!

ruflin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

glefloch commented May 1, 2017

Uh oh!

ruflin commented May 3, 2017

Uh oh!

urso commented May 11, 2017

Uh oh!

glefloch commented May 12, 2017

Uh oh!

elasticmachine commented Jun 5, 2017

Uh oh!

glefloch commented Jun 5, 2017

Uh oh!

tsg commented Jun 6, 2017

Uh oh!

glefloch commented Jun 6, 2017

Uh oh!

urso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glefloch commented Jun 9, 2017

Uh oh!

tsg commented Jun 12, 2017

Uh oh!

glefloch commented Jun 12, 2017

Uh oh!

glefloch commented Jun 14, 2017

Uh oh!

glefloch commented Jun 23, 2017

Uh oh!

urso commented Jun 26, 2017

Uh oh!

glefloch commented Jun 26, 2017

Uh oh!

dedemorton commented Aug 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

codecov bot commented May 1, 2017 •

edited

Loading