Add runtime metrics #1
Conversation
|
@noahfalk This is an initial stab at porting the runtime metrics from the contrib library. I've slightly tweaked to optimize the code, and I've been able to remove some redundant code that handles older .NET versions in the contrib codebase.
EDIT: Issue solved |
stevejgordon
force-pushed
the
runtime-metrics
branch
from
1548018 to
572bc2a
Compare
|
I think I figured out the above with some tweaks to the csproj file. Pushing those changes so we can discuss. |
...braries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/RuntimeMetrics.cs
Outdated
Show resolved
Hide resolved
...braries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/RuntimeMetrics.cs
Outdated
Show resolved
Hide resolved
|
Testing runtime metrics can be kinda tricky. I've added an initial POC for a test, although it might be a bit brittle at the moment as a GC could occur at any point during the test. Will harden that up if we feel tests like this are practical. |
There was a problem hiding this comment.
This looks pretty good overall! A few things top of mind:
- The checkin deadline for .NET 9 feature complete is next Thursday (the 18th) so we don't have much time left. Ideally we'd want to have a near final PR up this week to ensure some random CI failure or responding to a bit of PR feedback doesn't take this to the wire.
- I've been reviewing the semantic conventions PR and made a few more comments about some hopefully small changes.
- Tests are certainly nice, but if you have to make a tradeoff between getting this done before the deadline and having more/better tests, lets prioritize the deadline.
Thanks!
...ibraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MeterListener.cs
Show resolved
Hide resolved
| <Reference Include="System.Diagnostics.Tracing" /> | ||
| <Reference Include="System.Memory" /> | ||
| <Reference Include="System.Runtime" /> | ||
| <Reference Include="System.Runtime.InteropServices" /> | ||
| <Reference Include="System.Threading" /> | ||
| <Reference Include="System.Threading.Thread" /> | ||
| <Reference Include="System.Threading.ThreadPool" /> | ||
| <Reference Include="System.ComponentModel.Primitives" /> |
There was a problem hiding this comment.
just curious, what needed us to add ComponentModel.Primitives?
There was a problem hiding this comment.
It seems to be needed for Process.GetCurrentProcess()
...braries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/RuntimeMetrics.cs
Outdated
Show resolved
Hide resolved
...braries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/RuntimeMetrics.cs
Outdated
Show resolved
Hide resolved
|
|
||
| instrumentRecorder.RecordObservableInstruments(); | ||
|
|
||
| (bool success, IReadOnlyList<Measurement<long>> measurements) = await WaitForMeasurements(instrumentRecorder, GC.MaxGeneration + 1, token); |
There was a problem hiding this comment.
I'm not sure why we'd need to wait here? Other than exceptions it looks like every other metric is observable so I'd expect that all the measurements would have been received during the RecordObservableInstruments() call.
There was a problem hiding this comment.
On some runs, I saw the tests fail. I suspected that while RecordObservableInstruments returned and recorded everything, the OnMeasurementRecorded callback may not have queued them all at the point the code accessed the measurements. This solution might be a bit overengineered, though. A 50ms delay is probably sufficient to avoid this risk. I'll try that and we can see how it runs in CI.
stevejgordon
force-pushed
the
runtime-metrics
branch
from
f02da12 to
800c51a
Compare
Thanks for the feedback, @noahfalk. I've updated per your suggestions and added a bunch of extra tests. On my side, I think this is ready to go to a PR in the runtime, so I'll open it as a draft there so the CI can run. EDIT: See dotnet#104680 |
- Limits the new meter to .NET 9+ per the review feedback. - Comment out dotnet.process.cpu.time until blocking PR for a new API is merged. - Renames several counters to align with latest spec.
stevejgordon
force-pushed
the
runtime-metrics
branch
from
7faef8c to
6162a29
Compare
Ensure that each test only observes the expected Meter
* bug #1: don't allow for values out of the SerializationRecordType enum range * bug dotnet#2: throw SerializationException rather than KeyNotFoundException when the referenced record is missing or it points to a record of different type * bug dotnet#3: throw SerializationException rather than FormatException when it's being thrown by BinaryReader (or sth else that we use) * bug dotnet#4: document the fact that IOException can be thrown * bug dotnet#5: throw SerializationException rather than OverflowException when parsing the decimal fails * bug dotnet#6: 0 and 17 are illegal values for PrimitiveType enum * bug dotnet#7: throw SerializationException when a surrogate character is read (so far an ArgumentException was thrown)
WIP implementation for runtime metrics. Opening a PR in my fork to discuss initial technical details before this is ready for a final PR when the semantic conventions are stabilised.