[Metricbeat] new uWSGI module #6006
Conversation
|
Can one of the admins verify this patch? |
| } | ||
| } | ||
|
|
||
| func NewModule(base mb.BaseModule) (mb.Module, error) { |
There was a problem hiding this comment.
exported function NewModule should have comment or be unexported
metricbeat/module/uwsgi/testing.go
Outdated
| return env | ||
| } | ||
|
|
||
| func GetEnvHTTPServer() string { |
There was a problem hiding this comment.
exported function GetEnvHTTPServer should have comment or be unexported
metricbeat/module/uwsgi/testing.go
Outdated
|
|
||
| import "os" | ||
|
|
||
| func GetEnvTCPServer() string { |
There was a problem hiding this comment.
exported function GetEnvTCPServer should have comment or be unexported
| @@ -0,0 +1,7 @@ | |||
| - module: uwsgi | |||
| metricsets: ["status"] | |||
| enabled: false | |||
There was a problem hiding this comment.
Could you remove enabled here? we use the default now (true), as this is handled by modules subcommand
| period: 10s | ||
| hosts: ["tcp://127.0.0.1:9191"] | ||
|
|
||
|
|
There was a problem hiding this comment.
some spurious empty lines here
| type: long | ||
| description: > | ||
| Number of requests served | ||
| - name: core.static_requests |
There was a problem hiding this comment.
Sounds like you could group requests like: requests.total, requests.static, requests.routed, etc...
| } | ||
|
|
||
| // New creates a new instance of the MetricSet. | ||
| func New(base mb.BaseMetricSet) (mb.MetricSet, error) { |
There was a problem hiding this comment.
Could you add an Experimental warning here? Something like:
cfgwarn.Experimental("The uWSGI status metricset is experimental")
|
jenkins, test it |
|
Thanks @exekias for review, made requested changes |
| "read_errors": 0, | ||
| "pid": 1 | ||
| }, | ||
|
|
There was a problem hiding this comment.
Did you generate this file or add it manually? I kind of wonder how this newline here happened.
There was a problem hiding this comment.
I made this file manually, actually uwsgi.status contains only one of the nested fields (total, worker or core)
There was a problem hiding this comment.
Normally we generate these fields automatically with one of the tests like here for example: https://github.com/elastic/beats/blob/master/metricbeat/module/kibana/status/status_integration_test.go#L27 Like this we assure that the file stays up-to-date if we change mappings. Perhaps you could open a follow up PR with a file that is auto generated?
There was a problem hiding this comment.
This is great stuff. Thanks a lot for adding dashboards, tests etc.
Could you add a CHANGELOG entry?
I left some thoughts on the field naming. But I rather get this PR in and see how it works and do some potential adaptions in a follow up PR before we make it beta/GA.
| description: > | ||
| uwsgi.status metricset fields | ||
| fields: | ||
| - name: total.requests |
There was a problem hiding this comment.
I would probably turn around the namespacing as I thinkrequests is the namespace and total only describes which values it is in the requests namespace. Same below like:
errors.write.total
errors.read.total
and others.
There was a problem hiding this comment.
total was kind of summary for all workers, it has requests, exceptions, read, write errors, if I turnaround total.requests, can you suggest naming for summary data?
current structure is as below
"total": {
"requests": 102,
"exceptions": 1,
"write_errors": 0,
"read_errors": 0,
"pid": 1
}
turned around might look like this
"summary": {
"requests": {
"total": 102
},
"exceptions": 1,
"errors": {
"write": {
"total" : 0
},
"read": {
"total" : 0
},
}
"pid": 1
}
feels like to much overhead, but if it is considered as more readable I welcome the changes.
I appreciate if you suggest better structure for summary data.
There was a problem hiding this comment.
In general my approach is going from the generic one to the more detailed. So in the above case I think a user might first search for an error and then he can decide if writes or reads. I would assume he does not look at the document and think "I need a total" and then check what is inside total. But I'm pretty sure it could be argued the other way around :-)
|
jenkins, test it |
|
jenkins, test it please |
|
Merging it now, thank you so much for this awesome module! As @ruflin said, we can open follow up PRs to tweak field names |
This adds uWSGI server monitoring metrics.