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

Metrics Feature Switch #91767

Merged
merged 3 commits into from
Sep 12, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/workflow/trimming/feature-switches.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ configurations but their defaults might vary as any SDK can set the defaults dif
| EnableUnsafeBinaryFormatterSerialization | System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization | BinaryFormatter serialization support is trimmed when set to false |
| EventSourceSupport | System.Diagnostics.Tracing.EventSource.IsSupported | Any EventSource related code or logic is trimmed when set to false |
| InvariantGlobalization | System.Globalization.Invariant | All globalization specific code and data is trimmed when set to true |
| MetricsSupport | System.Diagnostics.Metrics.IsSupported | Any Metrics related code or logic is trimmed when set to false |
| PredefinedCulturesOnly | System.Globalization.PredefinedCulturesOnly | Don't allow creating a culture for which the platform does not have data |
| HybridGlobalization | System.Globalization.Hybrid | Properties connected with the mixed: platform-specific + icu-based globalization will be trimmed |
| UseSystemResourceKeys | System.Resources.UseSystemResourceKeys | Any localizable resources for system assemblies is trimmed when set to true |
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<linker>
<assembly fullname="System.Diagnostics.DiagnosticSource">
<type fullname="System.Diagnostics.Metrics.Meter" feature="System.Diagnostics.Metrics.IsSupported" featurevalue="false">
<method signature="System.Boolean get_IsSupported()" body="stub" value="false" />
</type>
<type fullname="System.Diagnostics.Metrics.Instrument" feature="System.Diagnostics.Metrics.IsSupported" featurevalue="false">
<method signature="System.Boolean get_Enabled()" body="stub" value="false" />
</type>
</assembly>
</linker>
Original file line number Diff line number Diff line change
Expand Up @@ -135,4 +135,8 @@ System.Diagnostics.DiagnosticSource</PackageDescription>
<PackageReference Include="System.Runtime.CompilerServices.Unsafe" Version="$(SystemRuntimeCompilerServicesUnsafeVersion)" />
</ItemGroup>

<ItemGroup>
<ILLinkSubstitutionsXmls Include="ILLink/ILLink.Substitutions.Shared.xml" />
</ItemGroup>

</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ protected Instrument(Meter meter, string name, string? unit, string? description
/// <param name="tags">A span of key-value pair tags associated with the measurement.</param>
protected void RecordMeasurement(T measurement, ReadOnlySpan<KeyValuePair<string, object?>> tags)
{
if (!Meter.IsSupported)
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid adding this check and count on _subscription.First to be null? It looks like you already blocked all the code paths that could have registered a subscription.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we can do that. According to my understanding (per @eerhardt explanation) jit is going to optimize that in tier two and will fully remove this check. I was hoping when IsSupported is false, the linker can empty the whole method body including accessing the static fields. Let me know if you still think we should remove this check.

Copy link
Member

Choose a reason for hiding this comment

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

Can you confirm the JIT does optimize it as expected. As long as that is confirmed then I have no qualms leaving it there :)

Copy link
Member

Choose a reason for hiding this comment

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

Adding this check will allow the trimmer to remove the rest of the method body, right?

Copy link
Member

Choose a reason for hiding this comment

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

Adding this check will allow the trimmer to remove the rest of the method body, right?

Correct - and all the methods it calls that aren't called elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

I created a simple app using my private built. The app and the runtime were built as a release. I have IsSupported = true to test the normal case. I run it under the debugger, and I am seeing the call Meter.IsSupported is not removed inside RecordMeasurement method. I'll remove this check from there as this is a hot path, we want to avoid regressions there.

image

{
return;
}

DiagNode<ListenerSubscription>? current = _subscriptions.First;
while (current is not null)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,12 @@ protected Instrument(Meter meter, string name, string? unit, string? description
/// </summary>
protected void Publish()
{
// All instruments call Publish when they are created. We don't want to publish the instrument if the Meter is not supported.
if (!Meter.IsSupported)
{
return;
}

List<MeterListener>? allListeners = null;
lock (Instrument.SyncObject)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,22 @@ public class Meter : IDisposable
private Dictionary<string, List<Instrument>> _nonObservableInstrumentsCache = new();
internal bool Disposed { get; private set; }

internal static bool IsSupported { get; } = InitializeIsSupported();

private static bool InitializeIsSupported() =>
AppContext.TryGetSwitch("System.Diagnostics.Metrics.IsSupported", out bool isSupported) ? isSupported : true;

/// <summary>
/// Initialize a new instance of the Meter using the <see cref="MeterOptions" />.
/// </summary>
public Meter(MeterOptions options)
{
if (!IsSupported)
{
Name = options is not null && options.Name is not null ? options.Name : throw new ArgumentNullException(nameof(options));
return;
}

if (options is null)
{
throw new ArgumentNullException(nameof(options));
Expand Down Expand Up @@ -61,6 +72,12 @@ public Meter(string name, string? version) : this (name, version, null, null) {}
/// </remarks>
public Meter(string name, string? version, IEnumerable<KeyValuePair<string, object?>>? tags, object? scope = null)
{
if (!IsSupported)
{
Name = name ?? throw new ArgumentNullException(nameof(name));
return;
}

Initialize(name, version, tags, scope);
Debug.Assert(Name is not null);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
namespace System.Diagnostics.Metrics
{
/// <summary>
/// A delegate to represent the Meterlistener callbacks used in measurements recording operation.
/// A delegate to represent the MeterListener callbacks used in measurements recording operation.
/// </summary>
public delegate void MeasurementCallback<T>(Instrument instrument, T measurement, ReadOnlySpan<KeyValuePair<string, object?>> tags, object? state) where T : struct;

Expand Down Expand Up @@ -56,6 +56,11 @@ public MeterListener() { }
/// <param name="state">A state object which will be passed back to the callback getting measurements events.</param>
public void EnableMeasurementEvents(Instrument instrument, object? state = null)
{
if (!Meter.IsSupported)
{
return;
}

bool oldStateStored = false;
bool enabled = false;
object? oldState = null;
Expand Down Expand Up @@ -92,6 +97,11 @@ public void EnableMeasurementEvents(Instrument instrument, object? state = null)
/// <returns>The state object originally passed to <see cref="EnableMeasurementEvents" /> method.</returns>
public object? DisableMeasurementEvents(Instrument instrument)
{
if (!Meter.IsSupported)
{
return default;
}

object? state = null;
lock (Instrument.SyncObject)
{
Expand All @@ -114,6 +124,11 @@ public void EnableMeasurementEvents(Instrument instrument, object? state = null)
/// <param name="measurementCallback">The callback which can be used to get measurement recording of numeric type T.</param>
public void SetMeasurementEventCallback<T>(MeasurementCallback<T>? measurementCallback) where T : struct
{
if (!Meter.IsSupported)
{
return;
}

measurementCallback ??= (instrument, measurement, tags, state) => { /* no-op */};

if (typeof(T) == typeof(byte))
Expand Down Expand Up @@ -155,6 +170,11 @@ public void SetMeasurementEventCallback<T>(MeasurementCallback<T>? measurementCa
/// </summary>
public void Start()
{
if (!Meter.IsSupported)
{
return;
}

List<Instrument>? publishedInstruments = null;
lock (Instrument.SyncObject)
{
Expand Down Expand Up @@ -184,6 +204,11 @@ public void Start()
/// </summary>
public void RecordObservableInstruments()
{
if (!Meter.IsSupported)
{
return;
}

List<Exception>? exceptionsList = null;
DiagNode<Instrument>? current = _enabledMeasurementInstruments.First;
while (current is not null)
Expand Down Expand Up @@ -215,6 +240,11 @@ public void RecordObservableInstruments()
/// </summary>
public void Dispose()
{
if (!Meter.IsSupported)
{
return;
}

Dictionary<Instrument, object?>? callbacksArguments = null;
Action<Instrument, object?>? measurementsCompleted = MeasurementsCompleted;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
</ItemGroup>
<ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
<Compile Include="DiagnosticSourceEventSourceBridgeTests.cs" />
<Compile Include="TestNotSupported.cs" />
</ItemGroup>
<ItemGroup>
<Compile Include="..\src\System\Diagnostics\Metrics\Aggregator.cs" Link="Aggregator.cs" />
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using Microsoft.DotNet.RemoteExecutor;
using System.Diagnostics.Metrics;
using Xunit;

namespace System.Diagnostics.Metrics.Tests
{
public class MetricsNotSupportedTest
{
/// <summary>
/// Tests using Metrics when the System.Diagnostics.Metrics.IsSupported
/// feature switch is set to disable all metrics operations.
/// </summary>
[ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
[InlineData(false)]
[InlineData(true)]
public void IsSupportedSwitch(bool value)
{
RemoteInvokeOptions options = new RemoteInvokeOptions();
options.RuntimeConfigurationOptions.Add("System.Diagnostics.Metrics.IsSupported", value);

RemoteExecutor.Invoke((val) =>
{
bool isSupported = bool.Parse(val);

Meter meter = new Meter("IsSupportedTest");
Counter<long> counter = meter.CreateCounter<long>("counter");
bool instrumentsPublished = false;
bool instrumentCompleted = false;
long counterValue = 100;

using (MeterListener listener = new MeterListener
{
InstrumentPublished = (instruments, theListener) => instrumentsPublished = true,
MeasurementsCompleted = (instruments, state) => instrumentCompleted = true
})
{
listener.EnableMeasurementEvents(counter, null);
listener.SetMeasurementEventCallback<long>((inst, measurement, tags, state) => counterValue = measurement);
listener.Start();

Assert.Equal(isSupported, counter.Enabled);

counter.Add(20);
}
meter.Dispose();

Assert.Equal(isSupported, instrumentsPublished);
Assert.Equal(isSupported, instrumentCompleted);
Assert.Equal(isSupported ? 20 : 100, counterValue);
}, value.ToString(), options).Dispose();
}
}
}