-
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 exchanges metricset #6955
Conversation
cd71e77
to
0c15e87
Compare
will #6606 also be added? |
Yes, the idea is to finish with both PRs |
Don't merge it yet, I want to try to generate a data.json with more metrics. |
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.
WFG
"durable": true, | ||
"internal": false, | ||
"messages": { | ||
"ack": {}, |
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.
Normally in case of empty objects they should not be added. Perhaps an issue in the schema? Not a blocker of the PR but worth mentioning.
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 haven't found how to avoid it, they are definned as optional schema.Object
s, there would be other way to define them?
"messages": c.Dict("message_stats", s.Schema{
"publish": s.Object{
"count": c.Int("publish", s.Optional),
"details": c.Dict("publish_details", s.Schema{
"rate": c.Float("rate"),
}, c.DictOptional),
},
...
}
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.
@ruflin would it be ok to define the schema with dotted field names?
"messages": c.Dict("message_stats", s.Schema{
"publish.count": c.Int("publish", s.Optional),
"publish.details": c.Dict("publish_details", s.Schema{
"rate": c.Float("rate"),
}, c.DictOptional),
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 wonder if this will have any affect on the output in the data.json?
For the empty objects: Can you try to set the surrounding doc as optional too? Just had a look at ack
and I think it's not set to optional.
0c15e87
to
072bb82
Compare
I have been playing around with rabbitmq, trying to understand these metrics, and it seems that for exchanges, only But according to documentation all metrics for queues, exchanges and channels fill the same I see two approaches here:
The advantage of the first option is that we'd be supporting any documented field if at some moment it is added to some specific object type, but probably if they are not now is because they don't make sense, for example, if queues and channels have the responsibility of redispatching, or controlling acks, probably exchanges are never going to have activity and metrics about that. I'd go for the second option, if only for the advantage of letting the users know what metrics to expect in each metricset. We can always revisit this decision if some new field is added in the future, but we avoid confusion by now. @CodingSpiderFox what do you think, does it make sense? could you check in your deployments if your exchanges have more metrics apart of the ones for |
@jsoriano I agree with your choice. Absolutely makes sense. I will check for the metrics in my deployments when I'm back to work |
b1889ac
to
be29327
Compare
Unfortunately, I can't check the deployments at the moment as I'm very busy with other stuff :/ |
Ok, I think this can be merged. It is added as beta and in principle it doesn't break any existing functionality in any case. @CodingSpiderFox please create a new issue, or add a coment here, if at some moment you can check the metrics in your deployments. |
Nice! |
be29327
to
51e862e
Compare
jenkins, test this again, please |
Continues with elastic#6607
Continues with elastic#6607
Which feature version of metricbeat does contain these new fields? |
@CodingSpiderFox these fields will be available in 6.4. |
Continues with #6607