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

Adding HAProxy module #2384

Merged
merged 23 commits into from Sep 8, 2016

Conversation

@hartfordfive
Copy link
Contributor

commented Aug 25, 2016

PR for issue #2247

hartfordfive added 11 commits Jun 15, 2016
Merge pull request #1 from elastic/master
Updating fork from remote master
Merge pull request #2 from elastic/master
Updating from remote master
@elasticsearch-release

This comment has been minimized.

Copy link

commented Aug 25, 2016

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
@elasticsearch-release

This comment has been minimized.

Copy link

commented Aug 25, 2016

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

This comment has been minimized.

Copy link
@ruflin

ruflin Aug 26, 2016

Collaborator

The logs directory shuld not be commited. it is under .gitignore in the master branch


for _, ln := range strings.Split(string(b), "\n") {

ln := strings.Trim(ln, " ")

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Aug 26, 2016

Member

There is the strings.TrimSpace function for this.

continue
}

parts := strings.Split(strings.Trim(ln, " "), ":")

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Aug 26, 2016

Member

ln has already been trimmed at this point.

var result *Info
err := mapstructure.Decode(resultMap, &result)
if err != nil {
panic(err)

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Aug 26, 2016

Member

Return the error rather than panic'ing.

}

// GetInfo returns the result from the 'show stat' command
func (c *Client) GetInfo() (infoRes *Info, err error) {

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Aug 26, 2016

Member

Don't use named return parameters unless you have good reason to. This applies to other parts of the code. https://github.com/golang/go/wiki/CodeReviewComments#named-result-parameters

General infomration collected on HAProxy process
fields:
- name: nb_proc
type: intger

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Aug 26, 2016

Member

s/intger/integer/

But instead of integer use long.

description: >
Current uptime in seconds
- name: mem_max_mb

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Aug 26, 2016

Member

Consider reporting this field as bytes rather than MB. Then you can apply a bytes field formatter so that Kibana renders the value nicely. If you do change it to bytes, then add format: bytes to this field. See this for an example: https://github.com/elastic/beats/blob/master/metricbeat/module/system/memory/_meta/fields.yml#L8

type: intger
description: >
- name: idle_pct

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Aug 26, 2016

Member

Consider making this value range on [0, 1] so you can use a float with a percentage formatter. For example: https://github.com/elastic/beats/blob/master/metricbeat/module/system/memory/_meta/fields.yml#L26-L27


hapc, err := haproxy.NewHaproxyClient(m.statsAddr)
if err != nil {
return nil, fmt.Errorf(fmt.Sprintf("HAProxy Client error: %s", err))

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Aug 26, 2016

Member

fmt.Errorf() accepts the same parameters as fmt.Sprintf so you can just drop the call to Sprintf.

if err != nil {
return response, err
}
c.connection = conn

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Aug 26, 2016

Member

Why store the connection in the Client if it's not going to be reused?

This comment has been minimized.

Copy link
@hartfordfive

hartfordfive Aug 27, 2016

Author Contributor

Good point. I've changed that.

@andrewkroh

This comment has been minimized.

Copy link
Member

commented Aug 26, 2016

@hartfordfive Looks like you are off to a great start. Thanks for the contribution.


var result *Info
err := mapstructure.Decode(resultMap, &result)
if err != nil {

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Sep 2, 2016

Member

Consider writing this which is slightly more concise. (not a mandatory change, it's just how I'd of written it)

if err := mapstructure.Decode(resultMap, &result); err != nil {
    return nil, err
}

This comment has been minimized.

Copy link
@hartfordfive

hartfordfive Sep 2, 2016

Author Contributor

Yes I definitely agree it's cleaner that way, I'll definitely change that.

- name: mem_max_mb
type: integer
format: bytes

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Sep 2, 2016

Member

If the value is actually in MB then you don't want to format it as bytes. I recommend converting the value to bytes and renaming the field so you can take advantage of the byte formatter in Kibana (it uses http://numeraljs.com/).

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Sep 2, 2016

Member

Oh, I see you do convert the value. So then I think the field just needs a rename to reflect the correct units.

// Convert this value to a float between 0.0 and 1.0
fval, err := strconv.ParseFloat(f.Interface().(string), 64)
if err != nil {
panic(err)

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Sep 2, 2016

Member

I don't recommend panic'ing. (https://github.com/golang/go/wiki/CodeReviewComments#dont-panic) Instead, either propagate the error or handle it.

func eventMapping(info []*haproxy.Stat) []common.MapStr {

var events []common.MapStr
source := map[string]interface{}{}

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Sep 2, 2016

Member

Doesn't look like this allocated map is ever used. You can remove it and declare source within the loop.


st := reflect.ValueOf(info).Elem()
typeOfT := st.Type()
source = map[string]interface{}{}

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Sep 2, 2016

Member

This is allocating a second map. I don't see a reason for it given the source was allocated above.

@andrewkroh

This comment has been minimized.

Copy link
Member

commented Sep 2, 2016

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).

@dedemorton

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2016

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!

@hartfordfive

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2016

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?

@andrewkroh

This comment has been minimized.

Copy link
Member

commented Sep 2, 2016

@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 make docs.

@hartfordfive

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2016

@andrewkroh I actually merged in the master and fixed the merge conflicts a few days ago. Should I do it again?

@hartfordfive

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2016

@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

This comment has been minimized.

Copy link
@ruflin

ruflin Sep 5, 2016

Collaborator

Not sure how this change neded up in this PR?

This comment has been minimized.

Copy link
@hartfordfive

hartfordfive Sep 5, 2016

Author Contributor

I believe I got these changes when merging in the latest master the other day into my branch.

This comment has been minimized.

Copy link
@ruflin

ruflin Sep 6, 2016

Collaborator

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 ?

@@ -0,0 +1,19 @@
{

This comment has been minimized.

Copy link
@ruflin

ruflin Sep 5, 2016

Collaborator

Which tool creates this file?

This comment has been minimized.

Copy link
@hartfordfive

hartfordfive Sep 5, 2016

Author Contributor

Sorry, I actually missed that file. The tool used was govendor. If you guys have another standard way of bringing in vendor dependencies, let me know and I'll do that instead.

@@ -0,0 +1,151 @@
Go CSV

This comment has been minimized.

Copy link
@ruflin

ruflin Sep 5, 2016

Collaborator

So far we added a vendor directory in the module directly if the dependency was specific to the module. TBH with csv it is fine from my point of view to have it in the top dependencies as this will be also used in other modules.

This comment has been minimized.

Copy link
@hartfordfive

hartfordfive Sep 5, 2016

Author Contributor

@ruflin I'm not exactly sure what I should change based on your comment?

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Sep 5, 2016

@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.

@hartfordfive

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2016

@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)

This comment has been minimized.

Copy link
@ruflin

ruflin Sep 6, 2016

Collaborator

errors.New would also work here. But that is a minor detail which can be tackled later.

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Sep 6, 2016

@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.

@ruflin ruflin merged commit de22fad into elastic:master Sep 8, 2016

3 checks passed

CLA Commit author has signed the CLA
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Sep 8, 2016

@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

claygorman added a commit to claygorman/beats that referenced this pull request Sep 8, 2016
suraj-soni added a commit to suraj-soni/beats that referenced this pull request Sep 20, 2016

@hartfordfive hartfordfive deleted the hartfordfive:metricbeat-haproxy branch May 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.