-
Notifications
You must be signed in to change notification settings - Fork 110
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
Multiple processes metrics #7200
base: main
Are you sure you want to change the base?
Multiple processes metrics #7200
Conversation
Following main repo changes
@dotnet-policy-service agree |
try | ||
{ | ||
DiagProcessFilter filter = DiagProcessFilter.FromConfiguration(_processFilterMonitor.CurrentValue); | ||
IEnumerable<IProcessInfo> processes = await _services.GetProcessesAsync(filter, stoppingToken); |
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 we'll need a separate filter for processes we monitor vs. processes we collect metrics for.
|
||
foreach (var completed in allCompleted) | ||
{ | ||
_store.RemoveMetricsForPid(completed.ProcessId); |
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.
Rather than keeping a list of tasks and checking their results, you could create continuations to StartMetricsPipelineForProcess that remove the pid from a set and remove the metrics from the store.
{ | ||
return new Dictionary<string, string>() | ||
{ | ||
{ "process_id", process.EndpointInfo.ProcessId.ToString() }, |
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.
These will likely need to be configurable
Thank you for your contribution. I put some initial feedback in the PR, but please hold off on additional changes until we figure out some of these design points:
|
Thank you for your feedback. I'll wait for your decisions.
I suppose some extensibility for "process label factory" could be introduced, similarly to egress extensibility model. |
Summary
This PR covers functionality requested in #2761 :
/metrics
endpoint. By default its value isfalse
and/metrics
endpoint behaves as currently: when multiple processes match default process filters, no data is returned. When set totrue
,/metrics
endpoint can return metrics of multiple processes, and:/metrics
endpoint will contain additional labels:process_name
andprocess_id
. Those labels identify process which is the source of given metricprocess_name
label returns name of app pool for IIS processes (that is, when system process name isw3wp
) and "regular" system process name for other processesRelease Notes Entry
Added option to track metrics of multiple processes via /metrics endpoint