Skip to content
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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Oct 10, 2022
Merged

[Instrumentation.Process] Implement threadSafe Refresh() #664

Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
d451059
initial commit
Yun-Ting Sep 28, 2022
5d688f1
comments
Yun-Ting Sep 28, 2022
b429d16
Merge branch 'main' into yunl/threadSafeRefresh
Yun-Ting Sep 29, 2022
13434f5
comments
Yun-Ting Oct 5, 2022
b97401f
Merge branch 'main' into yunl/threadSafeRefresh
Yun-Ting Oct 5, 2022
7646e7e
minor refactoring
Yun-Ting Oct 7, 2022
2c71223
Merge branch 'yunl/threadSafeRefresh' of https://github.com/Yun-Ting/…
Yun-Ting Oct 7, 2022
6bfac03
tests
Yun-Ting Oct 7, 2022
c27b0d9
fix
Yun-Ting Oct 10, 2022
ec9a430
name
Yun-Ting Oct 10, 2022
c11bf2d
Merge branch 'main' into yunl/threadSafeRefresh
utpilla Oct 10, 2022
ea77d50
update
Yun-Ting Oct 10, 2022
6a90e75
Merge branch 'yunl/threadSafeRefresh' of https://github.com/Yun-Ting/…
Yun-Ting Oct 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 48 additions & 18 deletions src/OpenTelemetry.Instrumentation.Process/ProcessMetrics.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,46 +16,76 @@

using System.Diagnostics.Metrics;
using System.Reflection;
using System.Threading;
using Diagnostics = System.Diagnostics;

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();

static ProcessMetrics()
private static readonly ThreadLocal<InstrumentsValues> CurrentThreadInstrumentsValues = new(() => new InstrumentsValues());

public ProcessMetrics(ProcessInstrumentationOptions options)
{
// TODO: change to ObservableUpDownCounter
MeterInstance.CreateObservableGauge(
"process.memory.usage",
() =>
{
CurrentProcess.Refresh();
return CurrentProcess.WorkingSet64;
},
() => CurrentThreadInstrumentsValues.Value.GetMemoryUsage(),
Yun-Ting marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this work with multiple MeterProvider setup?

In the scenario where you have multiple meterProviders and all of them add Process instrumentation, this would create the same set of instruments for each MeterProvider since they are all using the same Meter instance.

Copy link
Contributor Author

@Yun-Ting Yun-Ting Sep 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure for what intent would the user want to do .AddProcessInstrumentation() for multiple meterProviders to a given application/process. Everything in ProcessMetrics should ideally be static in the scope of a Process.

Could you share with me the use case you have in mind that ProcessMetrics was being added to more than one meterProvider?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One use case could be where the user wants to send data from different meters to different exporters along with the process metrics.

Copy link
Contributor Author

@Yun-Ting Yun-Ting Sep 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there will not be a problem if we mark the ctor of ProcessMetrics as static.
I still don't quite get why would the user want to monitor a static object via different exporters?
One case I could think of now is role-based access control - authorized users of the application can see a certain field like for example process.disk.io while non-authorized users cannot.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to use instance members for meter and InstrumentValues with this commit 13434f5.

One caveat with this implementation is in Snapshot(), threadA calls Process.Refresh(), and before threadA does the assignment to this.memoryUsage, threadB calls Process.Refresh(). The odds are small as assignments should be very fast.
Even if we hit this situation, it should not be a big deal because the time difference between these 2 refreshes is tiny.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One caveat with this implementation is in Snapshot(), threadA calls Process.Refresh(), and before threadA does the assignment to this.memoryUsage, threadB calls Process.Refresh(). The odds are small as assignments should be very fast. Even if we hit this situation, it should not be a big deal because the time difference between these 2 refreshes is tiny.

When would this happen? Is there a concrete example?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

The case I have in mind is:
At t3, the value got assigned to this.virtualMemoryUsage for threadA is from this.currentProcessRefresh() from t2 by threadB;
because GetCurrentProcess() is a static method that returns "A new Process component associated with the process resource that is running the calling application."

Even though we maintain different copies for currentProcess here:

opentelemetry-dotnet-contrib/src/OpenTelemetry.Instrumentation.Process/ProcessMetrics.cs

Line 51 in 13434f5

private readonly Diagnostics.Process currentProcess = Diagnostics.Process.GetCurrentProcess();

Those are referencing the same object.

But again, I don't think this is a case that could be hit frequently. And even it did occur, the time difference between the 2 snapshots are negligible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Snapshot is an instance method, under which condition would the same instance see concurrent calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah.... Snapshot() will not be accessed concurrently because it is an instance method.
What I was worried about is that whether those 2 instances are refreshing on the same object.

After further investigation, that is not the case:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this work with multiple MeterProvider setup?

In the scenario where you have multiple meterProviders and all of them add Process instrumentation, this would create the same set of instruments for each MeterProvider since they are all using the same Meter instance.

Please take a look:
https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/664/files#r989475625

unit: "By",
description: "The amount of physical memory in use.");

// TODO: change to ObservableUpDownCounter
MeterInstance.CreateObservableGauge(
"process.memory.virtual",
() =>
{
CurrentProcess.Refresh();
return CurrentProcess.VirtualMemorySize64;
},
() => CurrentThreadInstrumentsValues.Value.GetVirtualMemoryUsage(),
unit: "By",
description: "The amount of committed virtual memory.");
}

/// <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 sealed class InstrumentsValues
{
private static readonly Diagnostics.Process CurrentProcess = Diagnostics.Process.GetCurrentProcess();
private double? memoryUsage;
private double? virtualMemoryUsage;

internal InstrumentsValues()
{
this.memoryUsage = null;
Yun-Ting marked this conversation as resolved.
Show resolved Hide resolved
this.virtualMemoryUsage = null;
}

internal double GetMemoryUsage()
{
if (!this.memoryUsage.HasValue)
utpilla marked this conversation as resolved.
Show resolved Hide resolved
{
this.Snapshot();
}

var value = (double)this.memoryUsage;
Yun-Ting marked this conversation as resolved.
Show resolved Hide resolved
this.memoryUsage = null;
return value;
}

internal double GetVirtualMemoryUsage()
Yun-Ting marked this conversation as resolved.
Show resolved Hide resolved
{
if (!this.virtualMemoryUsage.HasValue)
{
this.Snapshot();
}

var value = (double)this.virtualMemoryUsage;
Yun-Ting marked this conversation as resolved.
Show resolved Hide resolved
this.virtualMemoryUsage = null;
return value;
}

private void Snapshot()
{
CurrentProcess.Refresh();
this.memoryUsage = CurrentProcess.WorkingSet64;
this.virtualMemoryUsage = CurrentProcess.PagedMemorySize64;
Yun-Ting marked this conversation as resolved.
Show resolved Hide resolved
}
}
}