Adding HAProxy module #2384
Adding HAProxy module #2384
Conversation
Updating fork from remote master
Updating from remote master
|
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. |
1 similar comment
|
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. |
| @@ -0,0 +1,10 @@ | |||
| 2016-08-10T19:06:01-04:00 INFO Metrics logging every 30s | |||
There was a problem hiding this comment.
The logs directory shuld not be commited. it is under .gitignore in the master branch
|
@hartfordfive Looks like you are off to a great start. Thanks for the contribution. |
| } | ||
|
|
||
| if parts[0] != "tcp" && parts[0] != "unix" { | ||
| return nil, errors.New("Invalid Protocol Scheme!") |
There was a problem hiding this comment.
Error strings should not be capitalized. https://github.com/golang/go/wiki/CodeReviewComments#error-strings
|
I looked over the Golang code and left a few comments. I didn't review the field names very closely to check if they conform to the event conventions, nor did I review the field documentation closely (maybe @dedemorton want to look at the docs, or maybe she prefers to edit them after they are merged). |
|
It's easier for me to edit the doc changes after they are merged. Please make sure you do a local doc build before the changes are merged to ensure that your contribution doesn't break the doc build. Many thanks! |
|
I've applied most of the modifications for the comments you mentioned above @andrewkroh although i still have to look into the event conventions. @dedemorton as for the local doc build, where can I see some details regarding that procedure? |
|
@hartfordfive Can you fix the merge conflicts. You probably need to rebase and address a few conflicts. Once updated I can test it on Jenkins which will also do a doc build so you don't have to. But if you want to test locally, the command is |
|
@andrewkroh I actually merged in the master and fixed the merge conflicts a few days ago. Should I do it again? |
|
@andrewkroh Ok I've done a few other changes and also updated the index templates to follow the updated event conventions. Let me know how it looks. |
| @@ -43,4 +43,6 @@ metricbeat.modules: | |||
| period: 10s | |||
| processes: ['.*'] | |||
|
|
|||
| # if true, exports the CPU usage in ticks, together with the percentage values | |||
There was a problem hiding this comment.
Not sure how this change neded up in this PR?
There was a problem hiding this comment.
I believe I got these changes when merging in the latest master the other day into my branch.
There was a problem hiding this comment.
Does not seem to be in master: https://github.com/elastic/beats/blob/master/metricbeat/etc/beat.yml Could you run git checkout master metricbeat/etc/beat.yml ?
|
@hartfordfive Thanks for all the work on this one. I think there are still parts we can tweak in the field naming but for all the other modules we also had here multiple iteration. I left a few very minor comments. The most important one for me is that we mark it experimental. See here https://github.com/elastic/beats/blob/master/metricbeat/module/beats/filebeat/filebeat.go#L26 as an example. This will allow us to change fields and structure with follow up PR's without someone expecting that this is already stable. From my point of view it would be ok to merge it soonish and then go through the hole module again and tweak it in a separate PR. But lets also get @andrewkroh view on this. |
|
@ruflin I've added the experimental warning messages as suggested. |
| @@ -65,13 +67,13 @@ func (m *MetricSet) Fetch() ([]common.MapStr, error) { | |||
|
|
|||
| hapc, err := haproxy.NewHaproxyClient(m.statsAddr) | |||
| if err != nil { | |||
| return nil, fmt.Errorf(fmt.Sprintf("HAProxy Client error: %s", err)) | |||
| return nil, fmt.Errorf("HAProxy Client error: %s", err) | |||
There was a problem hiding this comment.
errors.New would also work here. But that is a minor detail which can be tackled later.
|
@hartfordfive Thanks. Please check again my most recent comments above. I think it is important that we don't change our global defaults. Could be that these change happened sometimes during merging. |
|
@hartfordfive Merged. Thanks a lot for your contribution. I opened a follow up PR here to do some fixes like fixing the doc build: #2486 I also opened the github issue here to track some improvements that we have to do: #2485 |
PR for issue #2247