[Instrumentation.Process] Add schema URL#4088
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4088 +/- ##
==========================================
- Coverage 73.85% 73.82% -0.03%
==========================================
Files 459 459
Lines 18327 18307 -20
==========================================
- Hits 13535 13516 -19
+ Misses 4792 4791 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds OpenTelemetry schema URL (and scope version via MeterOptions.Version) to Process instrumentation metrics to align with the repo-wide effort in #4064.
Changes:
- Create the Process instrumentation
Metervia sharedMeterFactoryto populateTelemetrySchemaUrlandVersion. - Update
AddProcessInstrumentationto register the meter byMeterInstance.Name. - Extend unit tests to assert exported metrics include
MeterVersionand a schema URL prefix; update changelog.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/OpenTelemetry.Instrumentation.Process.Tests/ProcessMetricsTests.cs | Adds assertions validating MeterVersion and MeterSchemaUrl are present on exported metrics. |
| src/OpenTelemetry.Instrumentation.Process/ProcessMetrics.cs | Switches meter creation to shared factory with semantic conventions version to attach schema URL/version. |
| src/OpenTelemetry.Instrumentation.Process/OpenTelemetry.Instrumentation.Process.csproj | Links in shared MeterFactory.cs for this instrumentation. |
| src/OpenTelemetry.Instrumentation.Process/MeterProviderBuilderExtensions.cs | Registers the meter using ProcessMetrics.MeterInstance.Name. |
| src/OpenTelemetry.Instrumentation.Process/CHANGELOG.md | Documents the added scope version and schema URL behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e62e8f9 to
efb183a
Compare
The type was moved in main.
Move to unreleased.
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Kielek
left a comment
There was a problem hiding this comment.
It is not fully true.
process.cpu.count is not defined in the otel spec.
I think that we should drop it first and then merge it.
Ultimate solution, include metrics:
https://github.com/dotnet/runtime/blob/a534a1c68e656b07d16015e2f46fa55b25bfd0b9/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/RuntimeMetrics.cs#L143-L147
for .NET Fx + .NET8 in Runtime Instrumentation package + link changelog to use other packages during migration.
This comment was marked as outdated.
This comment was marked as outdated.
Remove the `process.cpu.count` metric which is not part of the Semantic Conventions.
Done. |
|
open-telemetry/semantic-conventions#651 is tracking the ask to make this part of the actual Semantic Conventions. |
|
On the other hand we have already https://github.com/open-telemetry/semantic-conventions/blob/main/docs/runtime/dotnet-metrics.md#metric-dotnetprocesscpucount I would not declare this metric as part of 1.25.0, as it is not defined there. |
It isn't because it's been deleted? If we don't emit it it isn't associated with that schema URL. |
Kielek
left a comment
There was a problem hiding this comment.
I missed that you have already committed fix here.
Contributes to #4064.
Changes
process.cpu.countmetric.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changesChanges in public API reviewed (if applicable)