[Metricbeat] Migrate HTTP json Metricset to use ReporterV2 interface #10960
Conversation
sayden
added
Metricbeat
sayden
changed the title
sayden
force-pushed
the
migration/mb/reporterv2/http/json
branch
from
b183a40
to
ca973ca
Compare
| @@ -24,6 +24,8 @@ import ( | |||
| "strconv" | |||
| "strings" | |||
|
|
|||
| "github.com/elastic/beats/libbeat/logp" | |||
|
|
|||
metricbeat/module/http/json/json.go
Outdated
| } | ||
| defer response.Body.Close() | ||
|
|
||
| var jsonBody common.MapStr | ||
| var jsonBodyArr []common.MapStr | ||
| var events []common.MapStr | ||
| //var events []common.MapStr |
metricbeat/module/http/json/json.go
Outdated
| } | ||
|
|
||
| return events, nil | ||
| return |
metricbeat/tests/system/test_http.py
Outdated
| @@ -33,10 +33,11 @@ def test_json(self): | |||
| self.assertEqual(len(output), 1) | |||
| evt = output[0] | |||
|
|
|||
| assert evt["http"]["test"]["hello"] == "world" | |||
| assert evt["http"]["json"]["hello"] == "world" | |||
There was a problem hiding this comment.
As the namespace is set to test, this should stay under test. Seems like your change does not respect this config anymore but should.
There was a problem hiding this comment.
Will this pass? Shouldn't this be under test?
metricbeat/module/http/json/json.go
Outdated
| } | ||
|
|
||
| for _, obj := range jsonBodyArr { | ||
| event := m.processBody(response, obj) | ||
| events = append(events, event) | ||
| reporter.Event(mb.Event{MetricSetFields: event}) |
There was a problem hiding this comment.
you need to report here the event under the configured namespace. Same below.
ruflin
added
in progress
|
I set this to |
|
Waiting for #11660 to be merged to rebase |
sayden
force-pushed
the
migration/mb/reporterv2/http/json
branch
from
ca973ca
to
7f9fcbb
Compare
sayden
force-pushed
the
migration/mb/reporterv2/http/json
branch
from
4918fc3
to
a22fa8c
Compare
| var event common.MapStr | ||
|
|
||
| if m.deDotEnabled { | ||
| event = common.DeDotJSON(jsonBody).(common.MapStr) |
There was a problem hiding this comment.
Not part of this PR so we should follow up but afterwards but I wonder if this type conversion is safe or if we should check it. Also 2 lines below.
There was a problem hiding this comment.
Do you want me to open a follow up PR to discuss it or better to leave it in an issue to discuss?
|
|
||
| return mb.Event{ | ||
| MetricSetFields: event, | ||
| Namespace: "http." + m.namespace, |
There was a problem hiding this comment.
Just a thought (not related to this PR): We should probably rename this to dataset instead of namespace as that is what it is in the event in the end.
metricbeat/module/http/json/json.go
Outdated
| events = append(events, event) | ||
|
|
||
| if reported := reporter.Event(event); !reported { | ||
| return errors.New("error reporting event") |
There was a problem hiding this comment.
This is related to the discussion we need to have in #11666
In general I don't think we should report an error here as it normally means the metricset or beat was stopped on purpose (which is not an error).
metricbeat/module/http/json/json.go
Outdated
| for _, h := range v { | ||
| value += h + " ," | ||
| if reported := reporter.Event(event); !reported { | ||
| return errors.Errorf("error reporting event: %#v", event) |
metricbeat/tests/system/test_http.py
Outdated
| @@ -33,10 +33,11 @@ def test_json(self): | |||
| self.assertEqual(len(output), 1) | |||
| evt = output[0] | |||
|
|
|||
| assert evt["http"]["test"]["hello"] == "world" | |||
| assert evt["http"]["json"]["hello"] == "world" | |||
There was a problem hiding this comment.
Will this pass? Shouldn't this be under test?
Refer to #10774 for more info
In this case, a slight change in system tests was needed. I'm not sure of what it was originally doing.