-
Notifications
You must be signed in to change notification settings - Fork 296
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
[Instrumentation.Process] Implement threadSafe Refresh() #664
Changes from all commits
d451059
5d688f1
b429d16
13434f5
b97401f
7646e7e
2c71223
6bfac03
c27b0d9
ec9a430
c11bf2d
ea77d50
6a90e75
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,42 +20,56 @@ | |
|
||
namespace OpenTelemetry.Instrumentation.Process; | ||
|
||
internal class ProcessMetrics | ||
internal sealed 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(); | ||
internal readonly Meter MeterInstance = new(AssemblyName.Name, AssemblyName.Version.ToString()); | ||
|
||
static ProcessMetrics() | ||
private readonly Diagnostics.Process currentProcess = Diagnostics.Process.GetCurrentProcess(); | ||
private double? memoryUsage; | ||
private double? virtualMemoryUsage; | ||
|
||
public ProcessMetrics(ProcessInstrumentationOptions options) | ||
{ | ||
// TODO: change to ObservableUpDownCounter | ||
MeterInstance.CreateObservableGauge( | ||
this.MeterInstance.CreateObservableGauge( | ||
"process.memory.usage", | ||
() => | ||
{ | ||
CurrentProcess.Refresh(); | ||
return CurrentProcess.WorkingSet64; | ||
if (!this.memoryUsage.HasValue) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For these callbacks to be thread-safe they need to be inside a lock. For example: () =>
{
lock(this)
{
if (!this.memoryUsage.HasValue)
{
this.Snapshot();
}
var value = this.memoryUsage.Value;
this.memoryUsage = null;
return value;
}
} |
||
{ | ||
this.Snapshot(); | ||
} | ||
|
||
var value = this.memoryUsage.Value; | ||
this.memoryUsage = null; | ||
return value; | ||
}, | ||
unit: "By", | ||
description: "The amount of physical memory in use."); | ||
description: "The amount of physical memory allocated for this process."); | ||
|
||
// TODO: change to ObservableUpDownCounter | ||
MeterInstance.CreateObservableGauge( | ||
this.MeterInstance.CreateObservableGauge( | ||
"process.memory.virtual", | ||
() => | ||
{ | ||
CurrentProcess.Refresh(); | ||
return CurrentProcess.VirtualMemorySize64; | ||
if (!this.virtualMemoryUsage.HasValue) | ||
{ | ||
this.Snapshot(); | ||
} | ||
|
||
var value = this.virtualMemoryUsage.Value; | ||
this.virtualMemoryUsage = null; | ||
return value; | ||
}, | ||
unit: "By", | ||
description: "The amount of committed virtual memory."); | ||
description: "The amount of virtual memory allocated for this process that cannot be shared with other processes."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was expecting "process.memory.virtual" to be Process.VirtualMemorySize64. I saw you linked to #673 which had a very nice explanation for why you made the change. I think you were attempting to be very thorough and precise in making this choice so I want to offer something equally detailed to give an alternate perspective on the problem. When we get into details, I think 'committed memory' is unfortunately an ambiguous term and the best way to understand what the OTel spec writers intended will be for us to ask them and clarify the spec. In Mark Russinovich's blog post he repeatedly refers to memory that "counts against the commit limit", but he never actually says "Memory is 'committed memory' if and only if it counts against the commit limit". I think that would be an intuitive definition in context of his article and probably it is the definition he intended the reader to apply despite never stating it pedantically. For simplicity we can just call that his intended definition. His definition is probably technically precise for how some portion of the Windows Kernel uses the term, but I suspect it is not how most people understand and use it. IMO the common definition of committed memory that most developers would use is that it is all the memory addresses a process can read from without triggering an access violation. This includes (at least) three categories of memory:
The first two agree with Mark's definition above so the difference is entirely whether the third bullet is included. To justify that it is common for most people to include file backed memory as part of 'committed memory' this article has a screenshot of the tool VMMap at the top. I consider VMMap to be the de-facto standard for investigating and categorizing virtual memory usage (incidentally its also a tool MarkR made). Looking at the table in that image is a column called 'Committed' and the first two rows are 'Image' and 'Mapped File'. If committed virtual memory did not include memory backed by files on disk then we'd expect those columns to be 0, but they aren't. As a second example, this link is a help page for the !address command in windbg which shows information about the virtual address space. There is an example output part way down the page showing:
The debugger says that this address is part of the kernel32 image and it is on a memory page that is marked both as being MEM_IMAGE and MEM_COMMIT. Mark's blog post says specifically that "The virtual memory in Testlimit's address space where its executable and system DLL images are mapped therefore don't count toward the commit limit" and yet windbg describes this exact type of memory as being MEM_COMMIT. I think the way VMMap and Windbg refer to committed memory is the way most developers would use the term. That definition corresponds to using Process.VirtualMemorySize64 as the metric value. Of course we can always ask the OTel spec writers if that is the definition they intended. Hope that helps! |
||
} | ||
|
||
/// <summary> | ||
/// Initializes a new instance of the <see cref="ProcessMetrics"/> class. | ||
/// </summary> | ||
/// <param name="options">The options to define the metrics.</param> | ||
public ProcessMetrics(ProcessInstrumentationOptions options) | ||
private void Snapshot() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you seen any practical examples where the lack of a snapshot caused trouble? For comparison, the CPU usage, GC memory usage, and total VM usage EventCounters have been present in .NET Core since .NET Core 3.1, there is nothing that forces them to be read as a single snapshot, and I am not aware of any user feedback saying that it has ever caused someone trouble. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressing the comment here: |
||
{ | ||
this.currentProcess.Refresh(); | ||
this.memoryUsage = this.currentProcess.WorkingSet64; | ||
this.virtualMemoryUsage = this.currentProcess.PrivateMemorySize64; | ||
} | ||
} |
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 wondering, if creating 2 instances of
ProcessMetrics
class, do we expect every instance create its own meter with the same name?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.
My understanding is that in the SDK, MetricStreamIdentity is maintained, so there will be only one
MetricStreamIdentity
in this case.What would happen is both of the callbacks from the instruments would be firing, but there will only be one export.
The second one will hit this https://github.com/open-telemetry/opentelemetry-dotnet/blob/7f71283c0bf2ee0e2c5e3c9617295ddaf20c7a0c/src/OpenTelemetry/Metrics/MetricReaderExt.cs#L51
Please refer to the testcases for the experiments I've done.
Thank you.
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 doubt that.
MetricStreamIdentity
is maintained at aMetricReader
level within the SDK. In the scenario, where we have two instances ofProcessMetrics
class due to multiple MeterProviders we would also have multiple readers each maintaining their own set ofMetricStreamIdentity
.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.
Hmm... I think each reader has its own storage with the current implementation - usage of instance for
ProcessMetrics
;so it may not be required to maintain MetricStreamIdentity at the reader level.