-
Notifications
You must be signed in to change notification settings - Fork 293
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] Added Cpu related metrics and addressed comments. #612
Conversation
@@ -21,4 +21,5 @@ namespace OpenTelemetry.Instrumentation.Process; | |||
/// </summary> | |||
public class ProcessInstrumentationOptions | |||
{ | |||
public bool? ExpandOnCpuStates { get; set; } |
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 strongly suggest to use a different name, and also strongly suggest to not ask me to provide one 🏃 😆
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.
Would CpuStatesEnabled be better? 😁
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 would vote for not having the option at all (and later if folks really need that, we can add it in a non-breaking way).
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.
So what should we have as the default behavior?
Breaking down by the states or not?
The spec is not very clear about this currently.
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.
What do you think? (I'll hold my answer for now 😄)
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.
Maybe default to not having the breakdown unless people really want it.
var priviledgedCpuTime = CurrentProcess.PrivilegedProcessorTime.Seconds; | ||
var userCpuTime = CurrentProcess.PrivilegedProcessorTime.Seconds; | ||
|
||
measurements[(int)CPUState.System] = new(priviledgedCpuTime, new KeyValuePair<string, object>("state", CPUState.System.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.
would we allocate a new string each time?
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.
yeah... that's the issue; maybe hardcoding the indices here is not that of a bad choice?
src/OpenTelemetry.Instrumentation.Process/ProcessInstrumentationOptions.cs
Outdated
Show resolved
Hide resolved
{ | ||
Measurement<long>[] measurements = new Measurement<long>[Enum.GetNames(typeof(CpuState)).Length]; | ||
var priviledgedCpuTime = this.currentProcess.PrivilegedProcessorTime.Seconds; | ||
var userCpuTime = this.currentProcess.UserProcessorTime.Seconds; |
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.
Do we want integer or double?
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.
Updated to use int.
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.
Why int?
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.
well, because the .Seconds
from the Timespan struct returns int?
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.
well, because the
.Seconds
from the Timespan struct returns int?
How is this related?
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.
Shouldn't it be .TotalSeconds
?
|
||
measurements[(int)CpuState.System] = new(priviledgedCpuTime, new KeyValuePair<string, object>("state", CpuState.System.ToString())); | ||
measurements[(int)CpuState.User] = new(userCpuTime, new KeyValuePair<string, object>("state", CpuState.User.ToString())); | ||
measurements[(int)CpuState.Wait] = new(this.currentProcess.TotalProcessorTime.Seconds - priviledgedCpuTime - userCpuTime, new KeyValuePair<string, object>("state", CpuState.Wait.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.
Will this give a non-zero value?
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.
The browser does not show me a very well-defined highlight for this comment. May I ask which part do you refer to?
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.
What does TotalProcessorTime - PriviledgedCpuTime - UserCpuTime
actually mean?
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.
If you read this https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.process.totalprocessortime?view=net-6.0#property-value:
TotalProcessorTime - PriviledgedCpuTime - UserCpuTim
is always going to give zero, unless .NET or the underlying native API has bugs.
src/OpenTelemetry.Instrumentation.Process/ProcessInstrumentationOptions.cs
Outdated
Show resolved
Hide resolved
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.
This looks heading towards a wrong direction.
Changes
Working towards: #447
Background: #335
Following OTel spec:
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/process-metrics.md#metric-instruments
Addressed some comments:
[Instrumentation.Process] Added memory related metrics. #595 (review)
[Instrumentation.Process] Added memory related metrics. #595 (comment)
Added CPU time and CPU utilization metrics. It is not clear from the spec that whether breakdown by CPU states (System, User, Wait) option should be implemented. Currently, this PR is not providing an option to specify that and uses no-breakdown gauge and counter by default.