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.

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.Runtime] Prefer the built-in runtime metrics for .NET 9 targets. #2339

Merged
merged 7 commits into from
Nov 22, 2024

Conversation

stevejgordon
Copy link
Contributor

Fixes #2071

Changes

Per #2071, this updates the instrumentation to prefer the built-in runtime metrics for .NET 9 targets.

Notes

I was unsure about adding the net9.0 target explicitly. Is there any prior art for dealing with features only available in specific targets that would be preferred?

Should we consider a flag (EnvVar) to disable the built-in metrics and instead prefer the original metrics? Currently, this would be a breaking change for consumers upgrading to .NET 9.0 as metric and attribute names differ. A further consideration is whether this should be opt-in rather than opt-out for those consumers.

In the current implementation, I've excluded much of the API surface in .NET 9.0 and, therefore, had to create public/unshipped API files per target framework. Is there a cleaner way to have all but net9.0 share the same files?

I've opened this PR as a draft while we clarify the approach for the above.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@stevejgordon stevejgordon changed the title Feature/2071 Prefer the built-in runtime metrics for .NET 9 targets. Nov 19, 2024
@github-actions github-actions bot requested review from twenzel and xiang17 November 19, 2024 12:19
@github-actions github-actions bot added the comp:instrumentation.runtime Things related to OpenTelemetry.Instrumentation.Runtime label Nov 19, 2024
@stevejgordon
Copy link
Contributor Author

FYI @Kielek

@stevejgordon stevejgordon requested a review from Kielek November 19, 2024 14:55
@Kielek Kielek changed the title Prefer the built-in runtime metrics for .NET 9 targets. [Instrumentation.Runtime] Prefer the built-in runtime metrics for .NET 9 targets. Nov 20, 2024
 - cover both method with new behaviour
 - update docs
 - update changelog entry
@Kielek
Copy link
Contributor

Kielek commented Nov 21, 2024

I was unsure about adding the net9.0 target explicitly. Is there any prior art for dealing with features only available in specific targets that would be preferred?

I do not think that it is worth to do this. For AutoInstrumentation we need to have this behavior in the runtime, so the compilation optimization is not necessary. We can provide it for specific request.

Should we consider a flag (EnvVar) to disable the built-in metrics and instead prefer the original metrics? Currently, this would be a breaking change for consumers upgrading to .NET 9.0 as metric and attribute names differ. A further consideration is whether this should be opt-in rather than opt-out for those consumers.

I do not think that we need it. PR is clear. We should relay on what is prepared by the .NET team.

I've opened this PR as a draft while we clarify the approach for the above.

I have added some direct fixes to your branch. Opening the PR for review.

@Kielek Kielek marked this pull request as ready for review November 21, 2024 11:59
@Kielek Kielek requested a review from a team as a code owner November 21, 2024 11:59
Copy link

codecov bot commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (71655ce) to head (2fb9ec7).
Report is 611 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             main     #2339       +/-   ##
============================================
+ Coverage   73.91%   100.00%   +26.08%     
============================================
  Files         267         2      -265     
  Lines        9615       145     -9470     
============================================
- Hits         7107       145     -6962     
+ Misses       2508         0     -2508     
Flag Coverage Δ
unittests-Instrumentation.Runtime 100.00% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...entation.Runtime/MeterProviderBuilderExtensions.cs 100.00% <100.00%> (ø)

... and 263 files with indirect coverage changes

---- 🚨 Try these New Features:

@Kielek Kielek merged commit 07f634d into open-telemetry:main Nov 22, 2024
60 checks passed
@stevejgordon stevejgordon deleted the feature/2071 branch November 22, 2024 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.runtime Things related to OpenTelemetry.Instrumentation.Runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Instrumentation.Runtime] .NET 9 updates and future plans
4 participants