Enhance RabbitMQ test to use real data and test with 3.6#10667
Enhance RabbitMQ test to use real data and test with 3.6#10667sayden merged 4 commits intoelastic:masterfrom
Conversation
ruflin
left a comment
There was a problem hiding this comment.
If we have new dependencies we need to add them with govendor to the repo as otherwise CI does not find them.
It seems the main objective of this PR is to generate a nicer data.json. For this to happen I wonder if we should better go the approach suggested here? #10648
There was a problem hiding this comment.
There is a check in the WriteEventsReporterV2 for the -data flag. If we want to skip it already here, we should do it based on the -data flag instead of always skipping it.
|
I'll take advantage of the new testing interface now on this PR |
43a8c2e to
30e39b0
Compare
There was a problem hiding this comment.
Error document as example? :-)
There was a problem hiding this comment.
Not sure if this is expected?
There was a problem hiding this comment.
Should we keep this around even though it's skipped. This is as a reminder that we should fix it.
2c22bae to
246cf5b
Compare
246cf5b to
38c94b6
Compare
38c94b6 to
5bc7e22
Compare
|
jenkins, test this please |
5bc7e22 to
718b00a
Compare
|
Updated the title to show the current state. |
jsoriano
left a comment
There was a problem hiding this comment.
Thanks for working on this.
Please update the compatibility notes mentioning that it should work with 3.6 too.
It'd be also good if we could test the node metricset with multiple versions, but not sure of the best way of doing it.
|
I passed this PR to ready again to wait until #12228 is in. Then I'll merge conflict and merge. |
6327d63 to
27a0b1f
Compare
|
Ok, conflicts solved :) Ready for review / merging EDIT: Ups, I forgot to update the compatibility docs. Doing it now. |
There was a problem hiding this comment.
Do we need to be more specific on "partially tested" here?
kaiyan-sheng
left a comment
There was a problem hiding this comment.
I'm just curious if we should be more specific on "partially tested". Other than that it looks good to me!
|
+1 on what @kaiyan-sheng says related to partial testing. Either we are confident it works and is supported or we should not mention it. Otherwise it's consuming for the customer on what to expect and also from a support perspective, if we support it or not. |
8e61fb3 to
79bc558
Compare
|
Text have been changed to reflect what's exactly tested with which version |
|
jenkins, test this please |
"Fixes" this issue #10014 by:
Currently, RabbitMQ module uses local data stored here to mock responses from the server during tests. This also generates file
data.jsonwhich is not very nice.AFAIK, the "missing data" reported in the issue are some fields that aren't parsed by design.
Current PR uses exchange Metricset to actively send messages and consume them from a RabbitMQ instance, to later produce a real
data.jsonfile.My main concern about this PR are:
Comments welcome :)
EDIT:
Finally I added some version specific tests using the new framework and updated to_error V2_ interface_. I couldn't update Metricset
nodeto use the new testing framework because it needs 2 HTTP calls to fetch metrics (and the testing framework it only with single HTTP calls at the moment)