[Metricbeat] Migrate Apache status Metricset to use ReporterV2 interface #10986
Conversation
sayden
added
Metricbeat
|
jenkins, test this |
sayden
force-pushed
the
migration/mb/reporterv2/apache/status
branch
from
f65ea9b
to
3767fa5
Compare
|
Error in Elasticsearch module seems unrelated https://beats-ci.elastic.co/job/elastic+beats+pull-request+multijob-linux/beat=metricbeat,label=linux-immutable/5506/console |
| @@ -55,7 +55,7 @@ def test_output(self): | |||
| time.sleep(0.5) | |||
|
|
|||
| proc = self.start_beat() | |||
| self.wait_until(lambda: self.output_lines() > 0) | |||
| self.wait_until(lambda: self.output_lines() > 0, max_timeout=20) | |||
There was a problem hiding this comment.
apache has a timeout of more then 10s?
There was a problem hiding this comment.
I can't remember why I changed this, frakly. I know there was a good reason 😕 Default is 10 seconds and I raised to 20 because I probably received some timeout in some moment. If you want I can leave it in 10 again until I remember 😅
There was a problem hiding this comment.
I prefer to go back to 10s as long as we don't know why.
ruflin
added
in progress
|
Same as other PR's, set it to |
sayden
force-pushed
the
migration/mb/reporterv2/apache/status
branch
from
3767fa5
to
9c9a5ef
Compare
|
@ruflin I tried to use the new testing framework here but I was getting a 404. I'm not sure about why so I uploaded everything I had progressed. |
|
@sayden Will have a look. |
| @@ -0,0 +1,3 @@ | |||
| type: http | |||
| url: "/server-status" | |||
There was a problem hiding this comment.
This needs to be url: "/server-status?auto=" as it uses a param.
| @@ -0,0 +1,354 @@ | |||
| <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 3.2 Final//EN"> | |||
There was a problem hiding this comment.
This should look identical to https://github.com/elastic/beats/blob/master/metricbeat/module/apache/status/_meta/test/status_apache-2.4.23-CentOS6 and the others. You missed probably the ?auto part in your query.
| @@ -55,7 +55,7 @@ def test_output(self): | |||
| time.sleep(0.5) | |||
|
|
|||
| proc = self.start_beat() | |||
| self.wait_until(lambda: self.output_lines() > 0) | |||
| self.wait_until(lambda: self.output_lines() > 0, max_timeout=20) | |||
There was a problem hiding this comment.
I prefer to go back to 10s as long as we don't know why.
sayden
force-pushed
the
migration/mb/reporterv2/apache/status
branch
3 times, most recently
from
1ced142
to
23ba063
Compare
sayden
force-pushed
the
migration/mb/reporterv2/apache/status
branch
from
23ba063
to
99ca5e6
Compare
sayden
added
review
and removed
in progress
There was a problem hiding this comment.
LGTM. We should remove the TestData either in this PR or a follow up.
I think quite a few of the status_test tests will also get obsolete with the change to the new testing framework. We should add apache tests files from different versions.
Ready to be merged, all the above can be done in follow up PR's. CI failure is not related.
|
|
||
| t.Logf("%s/%s event: %+v", f.Module().Name(), f.Name(), event) | ||
|
|
||
| // Check number of fields. | ||
| if len(event) < 11 { | ||
| if len(event.MetricSetFields) < 11 { | ||
| t.Fatal("Too few top-level elements in the event") | ||
| } | ||
| } | ||
|
|
||
| func TestData(t *testing.T) { |
Refer to #10774 for more info