-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rabbitmq queue metricset message rates #6606
Conversation
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left 2 comments. Thanks for splitting this up in separate PR's.
@@ -171,6 +171,11 @@ func Bool(key string, opts ...schema.SchemaOption) schema.Conv { | |||
return schema.SetOptions(schema.Conv{Key: key, Func: toBool}, opts) | |||
} | |||
|
|||
// Float creates a Conv object for parsing floats | |||
func Float(key string, opts ...schema.SchemaOption) schema.Conv { | |||
return schema.SetOptions(schema.Conv{Key: key, Func: toInteger}, opts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You use the toInteger
function here which I think does not return the expected result?
CHANGELOG.asciidoc
Outdated
@@ -256,6 +255,8 @@ https://github.com/elastic/beats/compare/v6.0.0-beta2...master[Check the HEAD di | |||
- Making the MongoDB module GA. {pull}6554[6554] | |||
- Allow to disable labels `dedot` in Docker module, in favor of a safe way to keep dots. {pull}6490[6490] | |||
- Add experimental module to collect metrics from munin nodes. {pull}6517[6517] | |||
- Add connections metricset to RabbitMQ module {pull}6548[6548] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should be removed for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can remove it, but currently this line is in the wrong section - Bugfixes, while it should be in Added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left the line in the change log, let me know if I should revert it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just realising now that this log line has been moved and not added.
+1 on moving it to the right section.
@@ -171,6 +171,38 @@ func Bool(key string, opts ...schema.SchemaOption) schema.Conv { | |||
return schema.SetOptions(schema.Conv{Key: key, Func: toBool}, opts) | |||
} | |||
|
|||
func toFloat(key string, data map[string]interface{}) (interface{}, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this function should be added to libbeat/common/coerce.go
, so it can be reused.
@kvalev branch needs rebase |
@kvalev: any updates? |
jenkins, test this please |
CHANGELOG.asciidoc
Outdated
@@ -193,6 +193,8 @@ https://github.com/elastic/beats/compare/v6.0.0-beta2...master[Check the HEAD di | |||
- Added support for haproxy 1.7 and 1.8. {pull}6793[6793] | |||
- Add accumulated I/O stats to diskio in the line of `docker stats`. {pull}6701[6701] | |||
- Ignore virtual filesystem types by default in system module. {pull}6819[6819] | |||
- Add connections metricset to RabbitMQ module {pull}6548[6548] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this line added on purpose? Was it missing in a previews PR? If yes, the PR number seems to be off.
@kvalev Could you rebase this PR again on master and fix the Changelog issue? |
I resolved the changelog conflict myself. I'm kind of surprised the mapstriface change does not conflict with #6870 |
jenkins, test this please |
What else is needed here? |
@CodingSpiderFox Could you rebase this again on master? |
I'm not good doing git stuff. Still learning. How can I do the rebase? Fork this repo, pull the branch kvalev and push it to my repo, rebase it on master and then open another PR? |
@CodingSpiderFox yes, this is what you'd need to do if you want to follow with these changes 🙂 |
jenkins, test this again please |
There is some issue with imports formatting. |
@CodingSpiderFox Sorry, my mistake. I somehow assumed you were the creator of the PR. |
I need this feature very soon |
Continuing with this PR in #6964 |
Rebased and merged as #6964 |
This pull requests extends the existing RabbitMQ queue metricset to include the message rates.
Related to #6442