-
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
Add metricbeat iis module #15059
Add metricbeat iis module #15059
Conversation
@jsoriano , I have touched on all the feedback, let me know if I miss anything |
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.
Thanks! Some more comments on the latest changes.
@@ -215,7 +215,7 @@ func getApplicationPools(names []string) ([]ApplicationPool, error) { | |||
return filtered, nil | |||
} | |||
|
|||
// getw3wpProceses func retrieves the running w3wp process ids | |||
// getw3wpProceses func retrieves the running w3wp process ids. A worker process is a windows process (w3wp.exe) which runs Web applications, and is responsible for handling requests sent to a Web Server for a specific application pool. |
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.
Nit. Very long line.
func init() { | ||
mb.Registry.MustAddMetricSet("windows", "perfmon", New) | ||
mb.Registry.MustAddMetricSet("windows", metricsetName, New) |
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 am thinking now that as these metricsets don't need a host because they only query perfmon, we could add this:
mb.Registry.MustAddMetricSet("windows", metricsetName, New) | |
mb.Registry.MustAddMetricSet("windows", metricsetName, New, | |
mb.WithHostParser(parse.EmptyHostParser), | |
) |
if err != nil { | ||
return nil, err | ||
} | ||
} |
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.
Oh ok, I would prefer if we can make it more explicit, like with an additional internal flag in the metricset.
Could it happen that we create another metricset based on perfmon where the instance label is also mandatory?
if err != nil { | ||
return nil, errors.Wrap(err, "failed formatting counter values") | ||
} | ||
var events []mb.Event | ||
// GroupAllCountersTo config option will only apply to the webserver metricset, where counters for all instances are aggregated | ||
if metricsetName != metricset && re.config.GroupAllCountersTo != "" { |
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.
Similarly here, I would prefer to don't depend on the metricset name for certain features, I would prefer this to be more explicit.
Also this generic metricset shouldn't have something specific for the webserver metricset (or any other metricset) if possible.
assert.NotNil(t, reader.Query.Handle) | ||
assert.NotNil(t, reader.Query.Counters) | ||
assert.Zero(t, len(reader.Query.Counters)) | ||
defer reader.close() |
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 comment is unaddressed (not so important in any case).
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.
Thanks! It LGTM, the only thing, that we could move process.pid
to the root level to follow ECS, but if it is complicated to do it now with the perfmon metricset we can leave it for a future change.
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.
Looks good. Suggested more metrics for the module.
measurement_label: total_connection_attempts | ||
query: '\Web Service(*)\Total Connection Attempts (all instances)' | ||
- instance_label: 'name' | ||
measurement_label: total_get_requests |
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.
@narph I can see total_get_requests
, total_post_requests
, total_delete_requests
.
Can we also pull total_put_requests
, total_head_requests
, total_trace_requests
, total_options_requests
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.
\Web Service(*)\Total PUT Requests
\Web Service(*)\PUT Requests/sec
\Web Service(*)\Total Head Requests
\Web Service(*)\Head Requests/sec
\Web Service(*)\Options Requests/sec
\Web Service(*)\Total Options Requests
\Web Service(*)\Other Request Methods/sec
\Web Service(*)\Total Other Request Methods
\Web Service(*)\Total Trace Requests
\Web Service(*)\Trace Requests/sec
\Web Service(*)\Total Unlock Requests
\Web Service(*)\Unlock Requests/sec
query: '\Web Service(_Total)\Bytes Received/sec' | ||
- instance_label: '' | ||
measurement_label: network.current_connections | ||
query: 'Web Service(_Total)\Current Connections' |
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.
Would be great to also have
Web Service(*)Current Anonymous Users
Web Service(*)Current NonAnonymous Users
Web Service(*)Total Anonymous Users
Web Service(*)Anonymous Users/sec
Web Service(*)Total NonAnonymous Users
query: '\Web Service(_Total)\Delete Requests/sec' | ||
- instance_label: '' | ||
measurement_label: network.service_uptime | ||
query: '\Web Service(_Total)\Service Uptime' |
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.
We can also add error counters:
Web Service(*)Total Not Found Errors
Web Service(*)Not Found Errors/sec
Web Service(*)Total Locked Errors
Web Service(*)Locked Errors/sec
* created iis module * work in progress * iis changes * add iis module * light * work on build * work on build * build * fmt update * temp * work on website * temp * temp * manifest changes * temp * temp * work on wp * adding application pool metricset * wmi option * test * work on apppool * work on app pool * work on website metricset * work on tests * Work on website * work on website * work on website * perfmon fix * work on websie * update config * feedback * work on feedback * temp * ecs * temp * add counters (cherry picked from commit 8be4589)
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.
Missing changelog here?
* Add metricbeat iis module (#15059) * created iis module * work in progress * iis changes * add iis module * light * work on build * work on build * build * fmt update * temp * work on website * temp * temp * manifest changes * temp * temp * work on wp * adding application pool metricset * wmi option * test * work on apppool * work on app pool * work on website metricset * work on tests * Work on website * work on website * work on website * perfmon fix * work on websie * update config * feedback * work on feedback * temp * ecs * temp * add counters (cherry picked from commit 8be4589) * fix file
Related issue #13072
Reusing the pdh query (in perfmon) to retrieve the iis specific counters.
Initial implementation contains 2 metricsets: