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] Remove gc.heap.fragmentation.size metrics due to buggy API on .NET 6 #509

Merged

Conversation

xiang17
Copy link
Contributor

@xiang17 xiang17 commented Jul 15, 2022

Removing this metrics because of the same issue #496. Unfortunately there is no convenient alternative API for this metric.

For significant contributions please make sure you have completed the following items:

@xiang17 xiang17 requested a review from a team as a code owner July 15, 2022 20:48
@codecov
Copy link

codecov bot commented Jul 15, 2022

Codecov Report

Merging #509 (43e30f9) into main (db3c2a2) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##            main    #509   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files        167     167           
  Lines       5098    5077   -21     
=====================================
+ Misses      5098    5077   -21     
Impacted Files Coverage Δ
...elemetry.Instrumentation.Runtime/RuntimeMetrics.cs 0.00% <ø> (ø)

@utpilla utpilla added the comp:instrumentation.runtime Things related to OpenTelemetry.Instrumentation.Runtime label Jul 18, 2022
Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add net7.0 target to the tests project and validate this as well?

@cijothomas cijothomas merged commit d9cc281 into open-telemetry:main Jul 19, 2022
@xiang17
Copy link
Contributor Author

xiang17 commented Jul 19, 2022

Could we add net7.0 target to the tests project and validate this as well?

I validated locally for .NET 7.0. However both the runtime library and the test project have to have net7.0 as target. If the runtime library doesn't have net7.0, the src\OpenTelemetry.Instrumentation.Runtime\bin\Debug folder won't have binaries for net7.0, and #if NET7_0_OR_GREATER is never true in any compiled binaries.

@xiang17 xiang17 deleted the xiang17/RemoveFragmentationSizeMetrics branch July 20, 2022 18:02
@cijothomas
Copy link
Member

Could we add net7.0 target to the tests project and validate this as well?

I validated locally for .NET 7.0. However both the runtime library and the test project have to have net7.0 as target. If the runtime library doesn't have net7.0, the src\OpenTelemetry.Instrumentation.Runtime\bin\Debug folder won't have binaries for net7.0, and #if NET7_0_OR_GREATER is never true in any compiled binaries.

Then its best to not rely on compiler preprocessor directives. You should switch to using Environment.Version to detect .NET 6 vs .NET 7, so that this can be used in .NET 7 apps right now.

@xiang17
Copy link
Contributor Author

xiang17 commented Jul 21, 2022

Could we add net7.0 target to the tests project and validate this as well?

I validated locally for .NET 7.0. However both the runtime library and the test project have to have net7.0 as target. If the runtime library doesn't have net7.0, the src\OpenTelemetry.Instrumentation.Runtime\bin\Debug folder won't have binaries for net7.0, and #if NET7_0_OR_GREATER is never true in any compiled binaries.

Then its best to not rely on compiler preprocessor directives. You should switch to using Environment.Version to detect .NET 6 vs .NET 7, so that this can be used in .NET 7 apps right now.

Added it back in #524.

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.

None yet

3 participants