-
Notifications
You must be signed in to change notification settings - Fork 283
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
[repo/Runtime] Prepare to .NET9 #2281
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2281 +/- ##
===========================================
+ Coverage 73.91% 98.02% +24.11%
===========================================
Files 267 2 -265
Lines 9615 152 -9463
===========================================
- Hits 7107 149 -6958
+ Misses 2508 3 -2505
Flags with carried forward coverage won't be shown. Click here to find out more.
|
#pragma warning disable SA1313 | ||
/// <summary> | ||
/// Initializes a new instance of the <see cref="RuntimeMetrics"/> class. | ||
/// </summary> | ||
/// <param name="options">The options to define the metrics.</param> | ||
public RuntimeMetrics(RuntimeInstrumentationOptions options) | ||
/// <param name="_1">The options to define the metrics.</param> | ||
public RuntimeMetrics(RuntimeInstrumentationOptions _1) | ||
#pragma warning restore SA1313 |
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.
A bit odd. Why not just remove the parameter?
-public RuntimeMetrics(RuntimeInstrumentationOptions _1)
+public RuntimeMetrics()
Could make the whole class static. But need some way to make sure it is bootstrapped.
Like instead of...
var instrumentation = new RuntimeMetrics(options);
builder.AddMeter(RuntimeMetrics.MeterInstance.Name);
return builder.AddInstrumentation(() => instrumentation);
Do something like...
GC.KeepAlive(RuntimeMetrics.SomethingAccessibleToTriggetStaticCtor)
return builder.AddMeter(RuntimeMetrics.MeterInstance.Name);
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.
Created #2289
The problem is with public contract if extension method. We need to keep this as the package is released as stable. Then we need to suppress warning there. I am not against, but lets do it seprately
public static MeterProviderBuilder AddRuntimeInstrumentation(
this MeterProviderBuilder builder,
Action<RuntimeInstrumentationOptions>? configure)
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.
LGTM
@@ -24,14 +24,16 @@ internal sealed class RuntimeMetrics | |||
#endif | |||
private const int NumberOfGenerations = 3; | |||
|
|||
private static readonly string[] GenNames = new string[] { "gen0", "gen1", "gen2", "loh", "poh" }; | |||
private static readonly string[] GenNames = ["gen0", "gen1", "gen2", "loh", "poh"]; | |||
#if NET |
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.
Is this #if NET
added by some automation tool?
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.
Nope, it was manual modification. isGcInfoAvailable
was marked as unused on some target frameworks. I manually modified it.
Changes
Follow up #2251
Merge requirement checklist
[ ] Unit tests added/updated[ ] AppropriateCHANGELOG.md
files updated for non-trivial changes[ ] Changes in public API reviewed (if applicable)