-
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
Closed
Closed
Changes from 21 commits
Commits
Show all changes
40 commits
Select commit
Hold shift + click to select a range
1a4d561
initial
Yun-Ting 654601a
AssemblyInfo
Yun-Ting 512dd21
Merge branch 'main' into yunl/pInst2
Yun-Ting 23a6d35
nit
Yun-Ting dedd949
nit
Yun-Ting aa9a818
CI
Yun-Ting 8a3e283
merge main
Yun-Ting 6a95d5b
initial
Yun-Ting 58f9dac
update
Yun-Ting 0b111bb
Merge branch 'main' into yunl/pInst2
Yun-Ting a1279b2
adding a new metric
Yun-Ting db31dac
convention
Yun-Ting 6bdd043
Merge branch 'yunl/pInst2' of https://github.com/Yun-Ting/opentelemet…
Yun-Ting 05d4478
semantics
Yun-Ting f1d267f
comments
Yun-Ting 12e12ba
Merge branch 'main' into yunl/pInst2
Yun-Ting 6f9d879
comment
Yun-Ting 07450d5
comment
Yun-Ting 1a63dc7
merge main
Yun-Ting b19a2f6
cpuTime
Yun-Ting d18d8c0
Merge branch 'main' into yunl/pInst4
cijothomas 371dbe3
nit
Yun-Ting 3a6c115
refresh once
Yun-Ting dc81215
ctor
Yun-Ting 9c6d2b0
param
Yun-Ting 57f2183
comments
Yun-Ting ef37cce
update
Yun-Ting 550966c
sanity
Yun-Ting 879bb9d
Oops
Yun-Ting 35c6186
comment
Yun-Ting ce98e93
unused
Yun-Ting 8e38272
update
Yun-Ting 10b5773
removed state option
Yun-Ting 893fbac
api
Yun-Ting 553efa4
added cpuUtil
Yun-Ting a24055f
update
Yun-Ting 7e9198b
Merge branch 'main' into yunl/pInst4
Yun-Ting e498b06
update
Yun-Ting 5167722
Merge branch 'yunl/pInst4' of https://github.com/Yun-Ting/opentelemet…
Yun-Ting e3fd84a
fix refresh
Yun-Ting File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ | |
// limitations under the License. | ||
// </copyright> | ||
|
||
using System.Collections.Generic; | ||
using System.Diagnostics.Metrics; | ||
using System.Reflection; | ||
using Diagnostics = System.Diagnostics; | ||
|
@@ -57,5 +58,74 @@ static ProcessMetrics() | |
/// <param name="options">The options to define the metrics.</param> | ||
public ProcessMetrics(ProcessInstrumentationOptions options) | ||
{ | ||
// TODO: change to ObservableUpDownCounter | ||
// MeterInstance.CreateObservableGauge( | ||
// $"process.cpu.utilization", | ||
// () => CurrentProcess.TotalProcessorTime.TotalMilliseconds / (Environment.ProcessorCount * (DateTime.Now - CurrentProcess.StartTime).Milliseconds), | ||
// unit: "1", | ||
// description: "Difference in process.cpu.time since the last measurement, divided by the elapsed time and number of CPUs available to the process."); | ||
|
||
// What type should be used for the label? | ||
// labels | ||
// state, if specified, SHOULD be one of: system, user, wait. A process SHOULD be characterized either by data points with no state labels, or only data points with state labels. | ||
// IEnumberable, | ||
// List<Measurement<long>> | ||
// wait time = totalprocessortime - Process.PrivilegedProcessorTime - Process.userProcessorTime | ||
|
||
if (options.ExpandOnCpuStates == true) | ||
{ | ||
MeterInstance.CreateObservableCounter( | ||
$"process.cpu.time", | ||
Yun-Ting marked this conversation as resolved.
Show resolved
Hide resolved
|
||
() => | ||
{ | ||
Measurement<long>[] measurements = new Measurement<long>[3]; | ||
|
||
CurrentProcess.Refresh(); | ||
var priviledgedCpuTime = CurrentProcess.PrivilegedProcessorTime.Seconds; | ||
var userCpuTime = CurrentProcess.PrivilegedProcessorTime.Seconds; | ||
Yun-Ting marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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 commentThe 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 commentThe 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? |
||
measurements[(int)CPUState.User] = new(userCpuTime, new KeyValuePair<string, object>("state", CPUState.User.ToString())); | ||
measurements[(int)CPUState.Wait] = new(CurrentProcess.TotalProcessorTime.Seconds - priviledgedCpuTime - userCpuTime, new KeyValuePair<string, object>("state", CPUState.Wait.ToString())); | ||
|
||
return measurements; | ||
}, | ||
unit: "s", | ||
description: "Total CPU seconds broken down by different states."); | ||
} | ||
else | ||
{ | ||
MeterInstance.CreateObservableCounter( | ||
$"process.cpu.time", | ||
() => | ||
{ | ||
CurrentProcess.Refresh(); | ||
return CurrentProcess.TotalProcessorTime.Seconds; | ||
}, | ||
unit: "s", | ||
description: "Total CPU seconds broken down by different states."); | ||
Yun-Ting marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
// TODO: change to ObservableUpDownCounter | ||
MeterInstance.CreateObservableGauge( | ||
"process.memory.usage", | ||
() => CurrentProcess.WorkingSet64, | ||
unit: "By", | ||
description: "The amount of physical memory in use."); | ||
|
||
// TODO: change to ObservableUpDownCounter | ||
MeterInstance.CreateObservableGauge( | ||
"process.memory.virtual", | ||
() => CurrentProcess.VirtualMemorySize64, | ||
unit: "By", | ||
description: "The amount of committed virtual memory."); | ||
|
||
} | ||
|
||
private enum CPUState | ||
{ | ||
System, | ||
User, | ||
Wait, | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.