[Instrumentation.Process] Added memory related metrics. #595
Conversation
Codecov Report
@@ Coverage Diff @@
## main #595 +/- ##
==========================================
+ Coverage 76.40% 76.70% +0.30%
==========================================
Files 170 170
Lines 5141 5160 +19
==========================================
+ Hits 3928 3958 +30
+ Misses 1213 1202 -11
|
utpilla
added
the
comp:instrumentation.process
Yun-Ting
changed the title
Yun-Ting
changed the title
test/OpenTelemetry.Instrumentation.Process.Tests/ProcessMetricsTests.cs
Outdated
Show resolved
Hide resolved
test/OpenTelemetry.Instrumentation.Process.Tests/ProcessMetricsTests.cs
Outdated
Show resolved
Hide resolved
| // TODO: change to ObservableUpDownCounter | ||
| MeterInstance.CreateObservableGauge( | ||
| "process.memory.usage", | ||
| () => CurrentProcess.WorkingSet64, |
There was a problem hiding this comment.
It's been awhile since I've studied the difference between WorkingSet64 and PrivateMemorySize64, so I'll admit I'm a bit fuzzy on the semantics of one over the other.
This seems to suggest that PrivateMemorySize64 may be useful as well https://stackoverflow.com/a/1986486/2052722.
There was a problem hiding this comment.
This seems to be a spec issue, currently the spec doesn't cover private bytes https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/process-metrics.md#metric-instruments.
There was a problem hiding this comment.
I would say all of these are useful (otherwise the NT OS kernel folks wouldn't invent these concepts):
I guess the struggle is do we want to just want to do a .NET specific thing (similar to what we ended up with https://github.com/open-telemetry/opentelemetry-dotnet-contrib/tree/main/src/OpenTelemetry.Instrumentation.Runtime) or try to align with / wait for the spec (or maybe help the spec to move forward). I can also see that the spec might struggle when it comes to some verbose memory management concepts because those could be OS specific (or even specific to certain versions of the NT kernel).
There was a problem hiding this comment.
Yes, I suspect this would be hard to generalize at the spec level. Maybe something like a process.dotnet.* namespace would be useful. Though, yea process.{os}.{os_distribution}.* might be more accurate but still hard to generalize across windows, linux, and whatever else.
There was a problem hiding this comment.
There was an effort a few years back to port these properties to equivalents for linux dotnet/corefx#41122. So at least for .NET and the distros of linux that it supports there should be some level of generalization across OSes.
There was a problem hiding this comment.
I'm not certain what the overhead of calling Refresh() for each metric is. If we find out it's expensive, we might consider this strategy in the future dotnet/runtime#62027 (comment).
Though... if Refresh is deemed cheap enough, then 👍 to keeping things simple.
|
|
||
| namespace OpenTelemetry.Instrumentation.Process; | ||
|
|
||
| internal class ProcessMetrics | ||
| { | ||
| internal static readonly AssemblyName AssemblyName = typeof(ProcessMetrics).Assembly.GetName(); | ||
| internal static readonly Meter MeterInstance = new(AssemblyName.Name, AssemblyName.Version.ToString()); | ||
| private static readonly Diagnostics.Process CurrentProcess = Diagnostics.Process.GetCurrentProcess(); |
There was a problem hiding this comment.
This, combined with Refresh, look like a source of race condition.
There was a problem hiding this comment.
@Yun-Ting You could probably just use a closure a la:
static ProcessMetrics()
{
Diagnostics.Process currentProcess = Diagnostics.Process.GetCurrentProcess();
// TODO: change to ObservableUpDownCounter
MeterInstance.CreateObservableGauge(
"process.memory.usage",
() =>
{
currentProcess.Refresh();
return currentProcess.WorkingSet64;
},
unit: "By",
description: "The amount of physical memory in use.");
// TODO: change to ObservableUpDownCounter
MeterInstance.CreateObservableGauge(
"process.memory.virtual",
() =>
{
currentProcess.Refresh();
return currentProcess.VirtualMemorySize64;
},
unit: "By",
description: "The amount of committed virtual memory.");
}Is Process.Refresh() expensive? We'll be invoking it twice here anytime metrics are collected. If it is expensive we might want to create a state object we can refresh once and then return the result to the two counters in the callback.
There was a problem hiding this comment.
Thank you very much for pointing it out. Please refer to #612 for the update.
There was a problem hiding this comment.
Please refer to the latest PR #624 instead. Thanks!
Please refer to the latest PR: #624 instead. Thanks! |
Changes
Working towards: #447
Background: #335
Adding 2 memory related metrics following the semantic conventions for OS process metrics as starters.
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/process-metrics.md#metric-instruments
Will be working on adding more metrics (cpu related and others) in the upcoming PRs.