Skip to content

[Metricbeat] Migrate Apache status Metricset to use ReporterV2 interface#10986

Merged
sayden merged 13 commits intoelastic:masterfrom
sayden:migration/mb/reporterv2/apache/status
Apr 11, 2019
Merged

[Metricbeat] Migrate Apache status Metricset to use ReporterV2 interface#10986
sayden merged 13 commits intoelastic:masterfrom
sayden:migration/mb/reporterv2/apache/status

Conversation

@sayden
Copy link
Copy Markdown
Contributor

@sayden sayden commented Feb 28, 2019

Refer to #10774 for more info

@sayden sayden added Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Feb 28, 2019
@sayden sayden self-assigned this Feb 28, 2019
@sayden sayden requested review from a team as code owners February 28, 2019 11:35
@sayden
Copy link
Copy Markdown
Contributor Author

sayden commented Mar 1, 2019

jenkins, test this

@sayden sayden force-pushed the migration/mb/reporterv2/apache/status branch from f65ea9b to 3767fa5 Compare March 1, 2019 12:19
@sayden
Copy link
Copy Markdown
Contributor Author

sayden commented Mar 5, 2019

@sayden sayden added the review label Mar 5, 2019
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apache has a timeout of more then 10s?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to go back to 10s as long as we don't know why.

@ruflin ruflin added in progress Pull request is currently in progress. and removed review labels Mar 13, 2019
@ruflin
Copy link
Copy Markdown
Contributor

ruflin commented Mar 13, 2019

Same as other PR's, set it to in progress.

@sayden sayden force-pushed the migration/mb/reporterv2/apache/status branch from 3767fa5 to 9c9a5ef Compare March 13, 2019 12:11
@sayden
Copy link
Copy Markdown
Contributor Author

sayden commented Mar 13, 2019

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

@ruflin
Copy link
Copy Markdown
Contributor

ruflin commented Mar 13, 2019

@sayden Will have a look.

Copy link
Copy Markdown
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sayden See my comments on how to fix the new data tests.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be url: "/server-status?auto=" as it uses a param.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to go back to 10s as long as we don't know why.

@sayden sayden force-pushed the migration/mb/reporterv2/apache/status branch 3 times, most recently from 1ced142 to 23ba063 Compare April 9, 2019 07:48
@sayden sayden force-pushed the migration/mb/reporterv2/apache/status branch from 23ba063 to 99ca5e6 Compare April 9, 2019 14:22
@sayden sayden added review and removed in progress Pull request is currently in progress. labels Apr 10, 2019
Copy link
Copy Markdown
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

func TestData(t *testing.T) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove this one, right?

@sayden sayden merged commit b9a3cdf into elastic:master Apr 11, 2019
@sayden sayden deleted the migration/mb/reporterv2/apache/status branch December 18, 2020 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Metricbeat Metricbeat review Team:Integrations Label for the Integrations team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants