Fix metricbeat http module not parsing headers correctly #8564 #8585

sayden · 2018-10-05T11:18:40Z

Refer to #8564 for more info

ruflin

Could you add a changelog entry? Seems like there is no system test generating that with headers and is the reason CI didn't detect it. Should we add one?

ruflin · 2018-10-05T11:21:47Z

metricbeat/module/http/_meta/fields.yml

@@ -14,7 +14,7 @@
          description: >
              HTTP request information
          fields:
-            - name: header
+            - name: headers


This also maps with packetbeat: https://github.com/elastic/beats/blob/master/packetbeat/protos/http/_meta/fields.yml#L17

Do you mean that it can produce a conflict? Means, I think I'm lacking context to understand your comment 😄

I meant it positive that it will not cause a conflict which is good.

sayden · 2018-10-17T13:19:44Z

@ruflin I think I need your review here. Also @jsoriano if you don't mind to take a look

ruflin · 2018-10-17T14:26:15Z

libbeat/tests/system/beat/beat.py

@@ -58,6 +58,8 @@ def start(self):
                stderr=subprocess.STDOUT,
                bufsize=0,
            )
+            # If a "No such file or directory" error points you here, run 


Good note, it's an error message I wanted to improve for some time.

Indeed these are good comments 🙂

What about handling these exceptions to print these messages so nobody has to look for these comments here? (It can be done in a future PR in any case)

Once it's merge, I'll open a new PR to address this errors and print them. It wasn't the good moment then and I'd like to understand better why the error happens instead of assuming some behavior and print some message that could be wrong and misleads the devs.

ruflin · 2018-10-17T14:27:34Z

metricbeat/tests/system/test_http.py

@@ -33,12 +41,15 @@ def test_json(self):
        self.assertEqual(len(output), 1)
        evt = output[0]

+        print(json.dumps(evt))


ruflin · 2018-10-17T14:27:55Z

metricbeat/tests/system/test_http.py

        assert evt["http"]["test"]["hello"] == "world"

        # Delete dynamic namespace part for fields comparison
        del evt["http"]["test"]

        self.assertItemsEqual(self.de_dot(HTTP_FIELDS), evt.keys(), evt)
+        print(json.dumps(evt))


Leftover? Did you plan to add here something around the new header you specified above?

ruflin · 2018-10-18T06:26:43Z

Can you rebase and take a look at my comments?

sayden · 2018-10-18T09:36:00Z

Rebase done and comments addressed. I was trying hard to improve the tests on this module but I was hitting the wall with some unknown errors. I'll do it in a different PR

ruflin

Changes LGTM. Needs rebase on top of master to pass ingest phase.

sayden · 2018-10-18T17:00:55Z

😕 rebase done but both tests complain about autopep8

ruflin · 2018-10-18T20:27:43Z

I assume if both complain, there is something off with the formatting. Try to run make fmt locally to see if you get a diff.

sayden · 2018-10-18T20:57:02Z

make fmt complains too about format here
https://github.com/elastic/beats/blob/master/metricbeat/module/elasticsearch/elasticsearch_integration_test.go#L129

and here
https://github.com/elastic/beats/blob/master/metricbeat/module/envoyproxy/server/data.go#L67

Which can be considered actual formatting errors, yes, because they aren't properly aligned.

sayden · 2018-10-18T21:26:47Z

When changing to 1.10.3 it seems that the formatter doesn't complain about those two files (althought I still think they aren't properly formatted ¯_(ツ)_/¯ )

Buuuuut tests still complain about autopep8

@ruflin you told me that you saw some diffs on a Python file so maybe it is something related with the Python version (2.7.10) instead of with Go version?

➜  metricbeat git:(metricbeat-http-headers-bugfix) go version
go version go1.10.3 darwin/amd64

➜  metricbeat git:(metricbeat-http-headers-bugfix) make clean && make fmt
New python executable in /Users/privacy/go/src/github.com/elastic/beats/metricbeat/build/python-env/bin/python
Installing setuptools, pip, wheel...done.

➜  metricbeat git:(metricbeat-http-headers-bugfix) . build/python-env/bin/activate

(python-env) ➜  metricbeat git:(metricbeat-http-headers-bugfix) make fmt

(python-env) ➜  metricbeat git:(metricbeat-http-headers-bugfix) python --version
Python 2.7.10

ruflin · 2018-10-19T07:25:48Z

I think the problem is that you are running make fmt instead the metricbeat directory but the diff is in libbeat. Best run make clean and make fmt in the beats directory so it is applied to all files.

My pyhton version is 2.7.15 but I don't think it makes a diff here because of the note above.

sayden · 2018-10-19T09:06:51Z

Running make clean && make fmt from the root and with Go 1.10.3 seems to finally format only the required file

ruflin · 2018-10-19T10:44:28Z

jenkins, test this

ruflin · 2018-10-22T08:29:00Z

metricbeat/module/http/_meta/test/main.go

+
+func serveHeader(w http.ResponseWriter, r *http.Request) {
+	fmt.Println("Key: " + r.Header.Get("key"))
+	fmt.Fprintf(w, `{"key":"%s"}`, r.Header.Get("key"))


This writes to the body of the http output instead of the headers I think?

sayden · 2018-10-22T13:35:19Z

@ruflin Changelog is added, although I'm not sure if there's some problem with Github yet because I have seen some conflict with CHANGELOG file which doesn't appear now.

ruflin · 2018-10-23T07:47:10Z

CHANGELOG.asciidoc

@@ -88,6 +88,8 @@ https://github.com/elastic/beats/compare/v6.4.0...master[Check the HEAD diff]
 - Avoid mapping issues in kubernetes module. {pull}8487[8487]
 - Fix issue that would prevent kafka module to find a proper broker when port is not set {pull}8613[8613]
 - Fix range colors in multiple visualizations. {issue}8633[8633] {pull}8634[8634]
+- Add Kafka dashboard. {pull}8457[8457]


This should not be here.

sayden added bug module Metricbeat Metricbeat needs_backport PR is waiting to be backported to other branches. labels Oct 5, 2018

ruflin suggested changes Oct 5, 2018

View reviewed changes

ruflin reviewed Oct 17, 2018

View reviewed changes

sayden added the review label Oct 17, 2018

sayden force-pushed the metricbeat-http-headers-bugfix branch from 26c0a57 to 0fd2644 Compare October 18, 2018 09:30

ruflin reviewed Oct 18, 2018

View reviewed changes

sayden force-pushed the metricbeat-http-headers-bugfix branch from 0fd2644 to 061c096 Compare October 18, 2018 16:55

ruflin reviewed Oct 22, 2018

View reviewed changes

sayden force-pushed the metricbeat-http-headers-bugfix branch from dd22a24 to 7001ba6 Compare October 22, 2018 09:44

Fix typo in headers naming

6ea540a

sayden force-pushed the metricbeat-http-headers-bugfix branch from 7001ba6 to 6ea540a Compare October 22, 2018 09:45

jsoriano approved these changes Oct 22, 2018

View reviewed changes

ruflin reviewed Oct 23, 2018

View reviewed changes

sayden added 2 commits October 23, 2018 10:30

Merge branch 'master' into metricbeat-http-headers-bugfix

18b8531

Remove Kafka mention from CHANGELOG

beda237

sayden force-pushed the metricbeat-http-headers-bugfix branch from b9d567b to beda237 Compare October 23, 2018 08:32

ruflin approved these changes Oct 23, 2018

View reviewed changes

sayden merged commit ace94f0 into elastic:master Oct 23, 2018

sayden mentioned this pull request Oct 23, 2018

Cherry-pick #8585 to 6.x: Fix metricbeat http module not parsing headers correctly #8564 #8692

Merged

sayden added v6.5.0 and removed needs_backport PR is waiting to be backported to other branches. labels Oct 23, 2018

sayden mentioned this pull request Dec 21, 2018

Bug in http.request/response.header beats vs template name #8564

Closed

sayden deleted the metricbeat-http-headers-bugfix branch August 25, 2022 12:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix metricbeat http module not parsing headers correctly #8564 #8585

Fix metricbeat http module not parsing headers correctly #8564 #8585

sayden commented Oct 5, 2018 •

edited

Loading

ruflin left a comment

ruflin Oct 5, 2018

sayden Oct 5, 2018 •

edited

Loading

ruflin Oct 5, 2018

sayden commented Oct 17, 2018

ruflin Oct 17, 2018

jsoriano Oct 18, 2018

sayden Oct 18, 2018

ruflin Oct 17, 2018

ruflin Oct 17, 2018

ruflin commented Oct 18, 2018

sayden commented Oct 18, 2018

ruflin left a comment

sayden commented Oct 18, 2018

ruflin commented Oct 18, 2018

sayden commented Oct 18, 2018

sayden commented Oct 18, 2018 •

edited

Loading

ruflin commented Oct 19, 2018

sayden commented Oct 19, 2018

ruflin commented Oct 19, 2018

ruflin Oct 22, 2018

sayden commented Oct 22, 2018

ruflin Oct 23, 2018

Fix metricbeat http module not parsing headers correctly #8564 #8585

Fix metricbeat http module not parsing headers correctly #8564 #8585

Conversation

sayden commented Oct 5, 2018 • edited Loading

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sayden Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sayden commented Oct 17, 2018

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

ruflin commented Oct 18, 2018

sayden commented Oct 18, 2018

ruflin left a comment

Choose a reason for hiding this comment

sayden commented Oct 18, 2018

ruflin commented Oct 18, 2018

sayden commented Oct 18, 2018

sayden commented Oct 18, 2018 • edited Loading

ruflin commented Oct 19, 2018

sayden commented Oct 19, 2018

ruflin commented Oct 19, 2018

Choose a reason for hiding this comment

sayden commented Oct 22, 2018

Choose a reason for hiding this comment

sayden commented Oct 5, 2018 •

edited

Loading

sayden Oct 5, 2018 •

edited

Loading

sayden commented Oct 18, 2018 •

edited

Loading