haproxy http stats #5819

jsoriano · 2017-12-06T02:08:31Z

Adds support for haproxy http stats endpoint with support with basic authentication (what fixes #5178).

elasticmachine · 2017-12-06T02:08:33Z

Can one of the admins verify this patch?

ruflin · 2017-12-06T02:24:46Z

@jsoriano Thanks a lot for you contribution. Any chance we could separate the PR into 2 PR's. One with the changes to the testing / Makefile and the other one with the Stat change. So we can discuss these changes seperately and one does not hold back the other.

jsoriano · 2017-12-06T02:25:47Z

@ruflin sure

ruflin · 2017-12-06T02:27:07Z

metricbeat/module/haproxy/haproxy.go

+}
+
+func (p *httpProto) Info() (*bytes.Buffer, error) {
+	return nil, errors.New("not supported")


That is interesting. So http does not support the info request? We should probably state that in the docs somewhere.

It seems so :/ I couldn't find a way to do it (apart of parsing the human stats html), and according to the code the dump info function is only called when executing the show info command https://github.com/haproxy/haproxy/blob/master/src/stats.c#L3130.

Ok, I'll review docs in the repo.

Doc and line to changelog added.

ruflin · 2017-12-06T02:28:37Z

metricbeat/tests/system/test_haproxy.py

-            "hosts": self.get_hosts(),
-            "period": "5s"
-        }])
+    def _test_info(self):


Why did you add an _ in front and remove the intergration part line?

I moved common code to "internal" functions so it can be reused for multiple cases. At the end this test has only the tcp socket case (it could maybe have also another case for a unix socket file), but the stat test has multiple cases.

Got it. The diff tricked me here :-)

jsoriano · 2017-12-06T02:31:21Z

Changes on tests moved to #5820

ruflin · 2017-12-06T02:32:08Z

Thanks a lot for taking this. Only had a quick look so far but that looks really promising. Will have a deeper look later.

ruflin

Could you add a line to the CHANGELOG.asciidoc and also update the docs file probably best the one on the module level that http and socket are supported, but only socket works for both metricsets?

ruflin · 2017-12-08T00:59:39Z

metricbeat/module/haproxy/haproxy.go

+func (p *httpProto) Stat() (*bytes.Buffer, error) {
+	url := p.URL.String()
+	// Force csv format
+	if !strings.HasSuffix(url, ";csv") {


What other formats do we have available?

Only human-oriented html, so csv is the only reasonable one to parse :)

JSON has been added to haproxy 1.8, but we may continue using CSV for backwards compatibility.

Sounds like something we should probably add more details about to the docs.

ruflin · 2017-12-08T01:02:05Z

metricbeat/module/haproxy/haproxy.go

+		return nil, fmt.Errorf("invalid response: %s", resp.Status)
+	}
+
+	d, err := ioutil.ReadAll(resp.Body)


I wonder if we need here afterwards resp.Body.Close() to not leak the resource?

Ouch, you are right, good catch

Close added

ruflin · 2017-12-08T01:03:46Z

metricbeat/tests/system/test_haproxy.py

-            "hosts": self.get_hosts(),
-            "period": "5s"
-        }])
+    def _test_info(self):


Got it. The diff tricked me here :-)

jsoriano · 2017-12-08T11:00:17Z

metricbeat/module/haproxy/haproxy.go

+	case "tcp":
+		return &Client{&unixProto{Network: u.Scheme, Address: u.Host}}, nil
+	case "unix":
+		return &Client{&unixProto{Network: u.Scheme, Address: u.Path}}, nil


I have modified this, before the url was parsed just splitting protocol and address, and the addres obtained this way was fine both for tcp and unix sockets. But now after the change to use url package for url parsing the address is Host for tcp, but Path for unix.

In any case, even if seems to be supported, there was no test coverage for unix socket, this will have to be revisited, as a volume in the docker would be needed to test this.

What are you saying is "this should still work the same but we don't have any tests and we didn't have any before"? :-)

ruflin

Some last minor comments. PR looks good to me.

ruflin · 2017-12-12T01:46:51Z

metricbeat/module/haproxy/_meta/docs.asciidoc

+For example, to enable stats reporting via any local IP on port 14567, place
+this statement under the `global` or `default` section of the haproxy config:
+
+ stats socket 127.0.0.1:14567


Please add ticks around this to make sure it shows up as code in the docs.

ruflin · 2017-12-12T01:47:57Z

metricbeat/module/haproxy/_meta/docs.asciidoc

+be added. For example, to open this frontend to any IP on port 14567 with
+required authentication add this to the haproxy config:
+
+ listen stats


Use a code block here:

["source","sh",subs="attributes"] ---- your code ----

I don't know what source type would work best here. @dedemorton perhaps knows?

It is rendered by github as code just leaving spaces before, the same with the previous snippet. Should I better use code blocks in any case?

Yes, please use code blocks as for our docs build Github is not really the one which counts in the end. We use code block in hopefully most other places.

I have set haproxy as "language", if empty, make docs fails.

ruflin · 2017-12-12T01:50:38Z

metricbeat/module/haproxy/haproxy.go

+	case "tcp":
+		return &Client{&unixProto{Network: u.Scheme, Address: u.Host}}, nil
+	case "unix":
+		return &Client{&unixProto{Network: u.Scheme, Address: u.Path}}, nil


What are you saying is "this should still work the same but we don't have any tests and we didn't have any before"? :-)

ruflin · 2017-12-12T02:05:28Z

metricbeat/module/haproxy/haproxy.go

+	}
+
+	if p.URL.User != nil {
+		password, _ := p.URL.User.Password()


I wonder if we should check this error here? Is no password = error?

The ignored value is a boolean telling if the password was set, I don't know if there are cases where basic authentication would work with an empty password... In any case I think it can be fine as is, if the credentials are wrong, the request will fail and this error is checked.

andrewkroh · 2017-12-12T02:59:05Z

metricbeat/module/haproxy/haproxy.go

 	if err != nil {
-		return response, err
+		return nil, errors.New("invalid url")


Use errors.Wrap so that the original error message is not lost.

andrewkroh · 2017-12-12T03:00:31Z

metricbeat/module/haproxy/haproxy.go

+	case "http", "https":
+		return &Client{&httpProto{URL: u}}, nil
+	default:
+		return nil, errors.New("invalid protocol scheme")


Include the invalid value in the error message with errors.Errorf.

Ok, using errors.Errorf in the rest of the file too.

andrewkroh · 2017-12-15T16:38:55Z

jenkins, test it

ruflin

LGTM. Only change needs is the code blocks in the docs.

ruflin · 2017-12-20T00:46:46Z

jenkins, test it

ruflin · 2017-12-20T00:47:40Z

@jsoriano Can you run make fmt and push again? That should make CI happy.

jsoriano · 2017-12-20T10:37:04Z

Pushed changes after make fmt and make collect-docs and CI seems happier :)

ruflin · 2017-12-21T00:25:45Z

jenkins, test it

ruflin · 2017-12-21T10:26:12Z

jenkins, test it

ruflin · 2017-12-27T04:25:03Z

@jsoriano Merged. Thanks for all your work on this one.

jsoriano force-pushed the haproxy-userpass branch from e1a5538 to 381455f Compare December 6, 2017 02:13

ruflin reviewed Dec 6, 2017

View reviewed changes

jsoriano force-pushed the haproxy-userpass branch from 381455f to d485360 Compare December 6, 2017 02:28

ruflin added Metricbeat Metricbeat review labels Dec 6, 2017

ruflin reviewed Dec 8, 2017

View reviewed changes

jsoriano force-pushed the haproxy-userpass branch from d485360 to b788c74 Compare December 8, 2017 10:41

jsoriano changed the title ~~haproxy http stats and some fixes in tests~~ haproxy http stats Dec 8, 2017

jsoriano commented Dec 8, 2017

View reviewed changes

ruflin reviewed Dec 12, 2017

View reviewed changes

andrewkroh reviewed Dec 12, 2017

View reviewed changes

jsoriano force-pushed the haproxy-userpass branch from b788c74 to 9e7a37e Compare December 12, 2017 14:22

andrewkroh approved these changes Dec 15, 2017

View reviewed changes

ruflin reviewed Dec 17, 2017

View reviewed changes

jsoriano force-pushed the haproxy-userpass branch from 9e7a37e to b259c12 Compare December 18, 2017 09:53

jsoriano force-pushed the haproxy-userpass branch 2 times, most recently from d6e73f2 to 5013a93 Compare December 20, 2017 09:27

jsoriano force-pushed the haproxy-userpass branch 2 times, most recently from 9fa6980 to b1e246a Compare December 21, 2017 10:07

jsoriano added 5 commits December 21, 2017 11:14

Haproxy module: Initial refactor and tests for http stats

c0b18c8

Haproxy http stats with basic authentication

4428b0d

Haproxy show info is not supported in http stats endpoint

56eb81c

Added documentation for HAproxy http stats frontend

8bfae65

Use errors library for errors in haproxy module

d2876f2

jsoriano force-pushed the haproxy-userpass branch from b1e246a to d2876f2 Compare December 21, 2017 10:14

ruflin merged commit 5792e5a into elastic:master Dec 27, 2017

ruflin mentioned this pull request Dec 27, 2017

HAProxy module authentication support for metrics #5178

Closed

ruflin mentioned this pull request Feb 26, 2018

Metricbeat compatibility with HAProxy 1.5 and 1.7 #5211

Closed

haproxy http stats #5819

haproxy http stats #5819

Conversation

jsoriano commented Dec 6, 2017 • edited Loading

elasticmachine commented Dec 6, 2017

ruflin commented Dec 6, 2017

jsoriano commented Dec 6, 2017

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

jsoriano commented Dec 6, 2017

ruflin commented Dec 6, 2017

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

jsoriano Dec 8, 2017 • edited Loading

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

jsoriano Dec 12, 2017 • edited 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

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

andrewkroh commented Dec 15, 2017

ruflin left a comment

Choose a reason for hiding this comment

ruflin commented Dec 20, 2017

ruflin commented Dec 20, 2017

jsoriano commented Dec 20, 2017

ruflin commented Dec 21, 2017

ruflin commented Dec 21, 2017

ruflin commented Dec 27, 2017

jsoriano commented Dec 6, 2017 •

edited

Loading

jsoriano Dec 8, 2017 •

edited

Loading

jsoriano Dec 12, 2017 •

edited

Loading