Read values from /proc/[pid]/status #41122

shubhamranjan · 2019-09-15T14:11:03Z

Add support for:

Process.PeakWorkingSet64
Process.PrivateMemorySize64
Process.PagedMemorySize64
Process.PagedSystemMemorySize64
Process.PeakVirtualMemorySize64

Improve:

Process.VirtualMemorySize64
Process.WorkingSet64

Fixes #23449
Fixes #36086

UPDATE: Fixes/Additions/ Supports have only been added on Linux. OSX is still unsupported.

shubhamranjan · 2019-09-15T14:54:17Z

I think I will have to modify the tests. Currently, since values are not being returned, they have been set to return 0.

src/Common/src/Interop/Linux/procfs/Interop.ProcFsStat.cs

shubhamranjan · 2019-09-19T18:55:28Z

This PR fixes an issue which exists in 2.1,2.2 and as well as 3.0. Is there something specific I need to do to make sure this fix makes its way for all supported versions ?

danmoseley · 2019-10-06T15:18:43Z

@krwq as area owner could you please help get this reviewed and merged?

danmoseley · 2019-10-06T15:21:18Z

Re : versions, this will not be back ported to shipped versions without clear customer need: we minimize churn in shipped releases . It might be a candidate for 3.1 if completed soon though.

shubhamranjan · 2019-10-06T15:27:05Z

Re : versions, this will not be back ported to shipped versions without clear customer need: we minimize churn in shipped releases . It might be a candidate for 3.1 if completed soon though.

I see. Currently, I find this required by most monitoring tools which, at the moment, on non-windows platforms are reporting memory usage as 0.

danmoseley · 2019-10-06T21:40:10Z

@shubhamranjan which monitoring tools? Is there a way to estimate their usage? How did these tools work before?

shubhamranjan · 2019-10-07T05:57:56Z

@shubhamranjan which monitoring tools? Is there a way to estimate their usage? How did these tools work before?

Newrelic is one of them. I am not sure about how many dotnet core customers use them. I have tested since 2.x versions of dotnet core and it never worked on non-windows platforms.

Ref: https://docs.newrelic.com/docs/agents/net-agent/installation/compatibility-requirements-net-core-agent#unavailable-features

danmoseley · 2019-10-07T19:17:57Z

Ah, that Newrelic documentation is a good source. Thank you.

2.1 is LTS so we are trying to minimise churn in it. 2.2 is going out of support soon because 3.0 shipped. 3.1 will ship soon: perhaps we can get this into there in the first instance, and see whether there is more interest in making the case for churn in 2.1.

cijothomas · 2019-10-21T18:17:00Z

@danmosemsft This is an issue for Azure Monitor/Application Insights as well. The memory is shown as 0 in Linux in Application Insights Live Metrics feature. It'd be good to get this fixed and ideally ported to 2.1/2.2 as well.

https://github.com/microsoft/ApplicationInsights-dotnet-server/issues/1299

danmoseley · 2019-10-21T18:33:28Z

@krwq could you please review so we can get this in master? we should keep it open to consider backport to 3.1.

danmoseley · 2019-10-21T20:09:16Z

@janvorli any concerns about consuming these values, and the mappings being made here? I know we've had various issues using the "correct" memory values on Linux in some cases.

src/Common/src/Interop/Linux/procfs/Interop.ProcFsStat.cs

src/System.Diagnostics.Process/tests/ProcessTests.cs

src/Common/src/Interop/Linux/procfs/Interop.ProcFsStat.cs

src/System.Diagnostics.Process/tests/ProcessTests.cs

stephentoub · 2019-11-01T16:17:06Z

src/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Linux.cs

-            return Interop.procfs.TryReadStatFile(pid, out stat, reusableReader) ?
-                CreateProcessInfo(ref stat, reusableReader) :
-                null;
+            return readStatFile || readStatusFile ? CreateProcessInfo(ref stat, ref status, reusableReader) : null;


readStatFile || readStatusFile

I don't understand this. How do we create the process info if we don't have a valid stat? We can do so without a valid status, but stat appears necessary, no? (e.g. for the pid, for the process name, etc.)

Pid is an input parameter, we already have that, so that's how we access /proc/[pid]/stat. Now what if reading /stat fails and /status doesn't ? You want to fail it as well ?

That's not how it's populated:

corefx/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Linux.cs

Line 129 in d391103

int pid = procFsStat.pid;

Note that pid currently isn't passed into CreateProcessInfo.

Now what if reading /stat fails and /status doesn't ? You want to fail it as well ?

Yup

That's not how it's populated:

corefx/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Linux.cs

Line 129 in d391103

int pid = procFsStat.pid;

Note that pid currently isn't passed into CreateProcessInfo.

Ah. Ok when you pass it forward, you read from the object itself.

shubhamranjan · 2019-11-01T20:51:13Z

@stephentoub Please re-review. I will be online for a while to do any changes if necessary on urgent basis if you guys plan to take this in your next release (since the cutoff is today I believe) .

danmoseley · 2019-11-02T00:01:13Z

@shubhamranjan thanks for working on this. At this point, we'll have to defer it from 3.1 I think. It may be possible to service it in later, if appropriate, we'll see. Meantime, let's carry on and get this into master.

shubhamranjan · 2019-11-02T05:47:07Z

@shubhamranjan thanks for working on this. At this point, we'll have to defer it from 3.1 I think. It may be possible to service it in later, if appropriate, we'll see. Meantime, let's carry on and get this into master.

@danmosemsft
Cool. Any plans for backporting this to 2.x and 3.0 ? I believe a lot of people wanting metrics would be using those versions and may have difficulty in upgrading to 3.1 asap.

danmoseley · 2019-11-02T06:25:30Z

Any plans for backporting this to 2.x and 3.0

2.2 and 3.0 will be out of support in not too long, obsoleted by 3.0 and 3.1 respectively. 2.1 is unlikely. That release has a very high bar for changes now.

stephentoub · 2019-11-04T03:50:21Z

src/Common/src/Interop/Linux/procfs/Interop.ProcFsStat.cs

+#endif
+                }
+
+                ReadOnlySpan<char> value = default;


Nit: this local is unnecessary... instead of e.g.

value = statusFileContents.Slice(0, nonUnitSliceLength); valueParsed = int.TryParse(value, out results.Pid);

it can be:

valueParsed = int.TryParse(statusFileContents.Slice(0, nonUnitSliceLength), out results.Pid);

stephentoub

Thanks!

stephentoub · 2019-11-04T03:53:05Z

@danmosemsft, up to you what you want to do about porting to 3.x.

* Fix #23449 - Read values from /proc/[pid]/status * Fix tests for new returned data from /prod/[pid]/status * Update tests of process info on OSX * Rename property ParsedStatus.pid --> ParsedStatus.Pid * 1. Avoid reallocation of delimiter array 2. Use tryParse instead of Parse to avoid exceptions * Rename static field and mark it as readonly * Change pid from /status to be read only from debug mode * Change status file read implementation using spans * Remove unecessary debug statement * Revert "Remove unecessary debug statement" This reverts commit f60b4f3. * Refactor logic using if statements with reduction in code redundancy * Improve condition arrangement to avoid reading subsequent files if process name doesn't match * Avoid failure of process info creation in case of fail read from one of many files. * Refactor Interop.ProcFsStat for better understanding * Change process creation logic to fail on read /stat * Change logic for unit and non unit values in file

danmoseley · 2019-11-04T18:44:51Z

src/Common/src/Interop/Linux/procfs/Interop.ProcFsStat.cs

+                    return true;
+                }
+            }
+            catch (IOException)


when reading /proc/<pid>/maps we also catch UnauthorizedAccessException . should we do that here?

In case you want a specific behaviour for UnauthorizedAccessException , we can add one.

* Fix #23449 - Read values from /proc/[pid]/status * Fix tests for new returned data from /prod/[pid]/status * Update tests of process info on OSX * Rename property ParsedStatus.pid --> ParsedStatus.Pid * 1. Avoid reallocation of delimiter array 2. Use tryParse instead of Parse to avoid exceptions * Rename static field and mark it as readonly * Change pid from /status to be read only from debug mode * Change status file read implementation using spans * Remove unecessary debug statement * Revert "Remove unecessary debug statement" This reverts commit f60b4f3. * Refactor logic using if statements with reduction in code redundancy * Improve condition arrangement to avoid reading subsequent files if process name doesn't match * Avoid failure of process info creation in case of fail read from one of many files. * Refactor Interop.ProcFsStat for better understanding * Change process creation logic to fail on read /stat * Change logic for unit and non unit values in file

cijothomas · 2019-11-25T18:16:53Z

@danmosemsft What version of .NET Core will this be part of ? 3.1 and higher only? Any scope of porting to 2.1/2.2 ?

danmoseley · 2019-11-25T21:19:00Z

@cijothomas it will be in 3.1 only. That is expected to be released next month. We don't plan to back port. 3.1 should be a suitable upgrade for most people, as it is expected to become a long term support release.

danmoseley · 2019-12-10T17:42:57Z

@cijothomas as you probably noticed, 3.1 is now released. Please let us know (in a new issue) if this does not work correctly for you.

cijothomas · 2019-12-11T23:04:56Z

@danmosemsft Yes! I validated this works and unblocks a key customer scenario for application insights customers.

danmoseley · 2019-12-11T23:38:37Z

Yay! Thanks @cijothomas for circling back. That makes it worth the rush to get this in.

@shubhamranjan you pointed to New Relic. Do you have a contact there who can update their documentation?

usr-sse2 · 2021-07-07T16:24:21Z

It seems to be working differently than on Windows.
On Windows I've never seen PrivateMemorySize64 differ from PagedMemorySize64 (my program throws an exception if it happens, and it haven't happened for two years).
With this change on Linux I see PrivateMemorySize64 6013288448 and PagedMemorySize64 8766259. From the code I see that paged on Linux shows only memory in a swap file.

The point of this assertion in my program is that I really need PeakPrivateMemorySize64, but there is no such field, and I use PeakPagedMemorySize64 instead because PagedMemorySize64 is always equal to PrivateMemorySize64.

Linux and Windows machines are identical and both have 32 GB RAM.

danmoseley · 2021-07-13T04:17:44Z

@usr-sse2 this PR is merged, so if you have an issue I suggest to open one in dotnet/runtime and link here if relevant.

* Fix dotnet/corefx#23449 - Read values from /proc/[pid]/status * Fix tests for new returned data from /prod/[pid]/status * Update tests of process info on OSX * Rename property ParsedStatus.pid --> ParsedStatus.Pid * 1. Avoid reallocation of delimiter array 2. Use tryParse instead of Parse to avoid exceptions * Rename static field and mark it as readonly * Change pid from /status to be read only from debug mode * Change status file read implementation using spans * Remove unecessary debug statement * Revert "Remove unecessary debug statement" This reverts commit dotnet/corefx@f60b4f3. * Refactor logic using if statements with reduction in code redundancy * Improve condition arrangement to avoid reading subsequent files if process name doesn't match * Avoid failure of process info creation in case of fail read from one of many files. * Refactor Interop.ProcFsStat for better understanding * Change process creation logic to fail on read /stat * Change logic for unit and non unit values in file Commit migrated from dotnet/corefx@8086f36

Dotnet-GitSync-Bot added the area-System.Diagnostics.Process label Sep 15, 2019

shubhamranjan marked this pull request as ready for review September 15, 2019 14:15

danmoseley reviewed Sep 15, 2019

View reviewed changes

src/Common/src/Interop/Linux/procfs/Interop.ProcFsStat.cs Outdated Show resolved Hide resolved

shubhamranjan requested a review from danmoseley September 15, 2019 17:03

danmoseley reviewed Sep 15, 2019

View reviewed changes

src/Common/src/Interop/Linux/procfs/Interop.ProcFsStat.cs Outdated Show resolved Hide resolved

shubhamranjan requested a review from danmoseley September 17, 2019 09:13

danmoseley requested a review from krwq October 6, 2019 15:18

maryamariyan requested a review from wtgodbe October 9, 2019 04:27

danmoseley added this to the 3.1 milestone Oct 21, 2019

krwq reviewed Oct 21, 2019

View reviewed changes

src/Common/src/Interop/Linux/procfs/Interop.ProcFsStat.cs Outdated Show resolved Hide resolved

krwq reviewed Oct 22, 2019

View reviewed changes

src/System.Diagnostics.Process/tests/ProcessTests.cs Show resolved Hide resolved

shubhamranjan added 4 commits October 22, 2019 23:11

Fix #23449 - Read values from /proc/[pid]/status

47f21e4

Fix tests for new returned data from /prod/[pid]/status

980181a

Update tests of process info on OSX

4948c04

Rename property ParsedStatus.pid --> ParsedStatus.Pid

4d81425

shubhamranjan requested a review from krwq October 22, 2019 19:36

krwq reviewed Oct 22, 2019

View reviewed changes

src/Common/src/Interop/Linux/procfs/Interop.ProcFsStat.cs Show resolved Hide resolved

krwq reviewed Oct 22, 2019

View reviewed changes

src/System.Diagnostics.Process/tests/ProcessTests.cs Show resolved Hide resolved

stephentoub reviewed Nov 1, 2019

View reviewed changes

shubhamranjan added 2 commits November 2, 2019 01:54

Refactor Interop.ProcFsStat for better understanding

20a3d70

Change process creation logic to fail on read /stat

2985dea

shubhamranjan requested a review from stephentoub November 1, 2019 20:49

Change logic for unit and non unit values in file

2967a46

stephentoub reviewed Nov 4, 2019

View reviewed changes

stephentoub approved these changes Nov 4, 2019

View reviewed changes

stephentoub merged commit 8086f36 into dotnet:master Nov 4, 2019

danmoseley reviewed Nov 4, 2019

View reviewed changes

danmoseley mentioned this pull request Nov 4, 2019

[release/3.1] Read values from /proc/[pid]/status (#41122) #42370

Merged

karelz modified the milestones: 3.1, 5.0 Dec 19, 2019

alanwest mentioned this pull request Aug 25, 2022

[Instrumentation.Process] Added memory related metrics. open-telemetry/opentelemetry-dotnet-contrib#595

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read values from /proc/[pid]/status #41122

Read values from /proc/[pid]/status #41122

shubhamranjan commented Sep 15, 2019 •

edited

Loading

shubhamranjan commented Sep 15, 2019

shubhamranjan commented Sep 19, 2019

danmoseley commented Oct 6, 2019

danmoseley commented Oct 6, 2019

shubhamranjan commented Oct 6, 2019

danmoseley commented Oct 6, 2019

shubhamranjan commented Oct 7, 2019

danmoseley commented Oct 7, 2019

cijothomas commented Oct 21, 2019

danmoseley commented Oct 21, 2019

danmoseley commented Oct 21, 2019

stephentoub Nov 1, 2019 •

edited

Loading

shubhamranjan Nov 1, 2019

stephentoub Nov 1, 2019 •

edited

Loading

stephentoub Nov 1, 2019

shubhamranjan Nov 1, 2019

shubhamranjan Nov 1, 2019

shubhamranjan commented Nov 1, 2019 •

edited

Loading

danmoseley commented Nov 2, 2019

shubhamranjan commented Nov 2, 2019

danmoseley commented Nov 2, 2019

stephentoub Nov 4, 2019

stephentoub left a comment

stephentoub commented Nov 4, 2019

danmoseley Nov 4, 2019

shubhamranjan Nov 5, 2019

cijothomas commented Nov 25, 2019

danmoseley commented Nov 25, 2019

danmoseley commented Dec 10, 2019

cijothomas commented Dec 11, 2019

danmoseley commented Dec 11, 2019

usr-sse2 commented Jul 7, 2021 •

edited

Loading

danmoseley commented Jul 13, 2021

Read values from /proc/[pid]/status #41122

Read values from /proc/[pid]/status #41122

Conversation

shubhamranjan commented Sep 15, 2019 • edited Loading

shubhamranjan commented Sep 15, 2019

shubhamranjan commented Sep 19, 2019

danmoseley commented Oct 6, 2019

danmoseley commented Oct 6, 2019

shubhamranjan commented Oct 6, 2019

danmoseley commented Oct 6, 2019

shubhamranjan commented Oct 7, 2019

danmoseley commented Oct 7, 2019

cijothomas commented Oct 21, 2019

danmoseley commented Oct 21, 2019

danmoseley commented Oct 21, 2019

stephentoub Nov 1, 2019 • edited Loading

Choose a reason for hiding this comment

shubhamranjan Nov 1, 2019

Choose a reason for hiding this comment

stephentoub Nov 1, 2019 • edited Loading

Choose a reason for hiding this comment

stephentoub Nov 1, 2019

Choose a reason for hiding this comment

shubhamranjan Nov 1, 2019

Choose a reason for hiding this comment

shubhamranjan Nov 1, 2019

Choose a reason for hiding this comment

shubhamranjan commented Nov 1, 2019 • edited Loading

danmoseley commented Nov 2, 2019

shubhamranjan commented Nov 2, 2019

danmoseley commented Nov 2, 2019

stephentoub Nov 4, 2019

Choose a reason for hiding this comment

stephentoub left a comment

Choose a reason for hiding this comment

stephentoub commented Nov 4, 2019

danmoseley Nov 4, 2019

Choose a reason for hiding this comment

shubhamranjan Nov 5, 2019

Choose a reason for hiding this comment

cijothomas commented Nov 25, 2019

danmoseley commented Nov 25, 2019

danmoseley commented Dec 10, 2019

cijothomas commented Dec 11, 2019

danmoseley commented Dec 11, 2019

usr-sse2 commented Jul 7, 2021 • edited Loading

danmoseley commented Jul 13, 2021

shubhamranjan commented Sep 15, 2019 •

edited

Loading

stephentoub Nov 1, 2019 •

edited

Loading

stephentoub Nov 1, 2019 •

edited

Loading

shubhamranjan commented Nov 1, 2019 •

edited

Loading

usr-sse2 commented Jul 7, 2021 •

edited

Loading