Bogus null checks in MeterListener.SetMeasurementEventCallback<T>

[KalleOlaviNiemitalo]

Description

If the public void SetMeasurementEventCallback<T>(MeasurementCallback<T>? measurementCallback) method of System.Diagnostics.Metrics.MeterListener is called with a null callback, then it throws an InvalidOperationException that claims that the type is unsupported, even if MeterListener actually supports the type.

Reproduction Steps

using System.Diagnostics.Metrics;

using var listener = new MeterListener();
listener.SetMeasurementEventCallback<int>(null);

Expected behavior

listener.SetMeasurementEventCallback<int>(null); should discard any previously set callback for int and succeed.

Actual behavior

Self-contradictory exception:

Unhandled exception. System.InvalidOperationException: System.Int32 is unsupported type for this operation. The only supported types are byte, short, int, long, float, double, and decimal.
   at System.Diagnostics.Metrics.MeterListener.SetMeasurementEventCallback[T](MeasurementCallback`1 measurementCallback)
   at Program.<Main>$(String[] args)

Regression?

Not a regression; the original implementation in #52685 already did this.

Known Workarounds

Provide a no-op callback instead of null.

Configuration

.NET SDK 6.0.202, runtime 6.0.4. Windows 10 Version 21H2 on x64.
It is not specific to this configuration.

Other information

All the measurementCallback is null checks in MeterListener.SetMeasurementEventCallback are currently redundant because they are only evaluated if a condition like measurementCallback is MeasurementCallback<byte> byteCallback matches, which can't happen if measurementCallback is null. Perhaps the method should instead check typeof(T) == typeof(byte) etc. and then cast (MeasurementCallback<byte>)(object)measurementCallback.

runtime/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MeterListener.cs

Lines 113 to 152 in be98e88

	/// <summary>
	/// Sets a callback for a specific numeric type to get the measurement recording notification from all instruments which enabled listening and was created with the same specified numeric type.
	/// If a measurement of type T is recorded and a callback of type T is registered, that callback will be used.
	/// </summary>
	/// <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 (measurementCallback is MeasurementCallback<byte> byteCallback)
	{
	_byteMeasurementCallback = (measurementCallback is null) ? ((instrument, measurement, tags, state) => { /* no-op */}) : byteCallback;
	}
	else if (measurementCallback is MeasurementCallback<int> intCallback)
	{
	_intMeasurementCallback = (measurementCallback is null) ? ((instrument, measurement, tags, state) => { /* no-op */}) : intCallback;
	}
	else if (measurementCallback is MeasurementCallback<float> floatCallback)
	{
	_floatMeasurementCallback = (measurementCallback is null) ? ((instrument, measurement, tags, state) => { /* no-op */}) : floatCallback;
	}
	else if (measurementCallback is MeasurementCallback<double> doubleCallback)
	{
	_doubleMeasurementCallback = (measurementCallback is null) ? ((instrument, measurement, tags, state) => { /* no-op */}) : doubleCallback;
	}
	else if (measurementCallback is MeasurementCallback<decimal> decimalCallback)
	{
	_decimalMeasurementCallback = (measurementCallback is null) ? ((instrument, measurement, tags, state) => { /* no-op */}) : decimalCallback;
	}
	else if (measurementCallback is MeasurementCallback<short> shortCallback)
	{
	_shortMeasurementCallback = (measurementCallback is null) ? ((instrument, measurement, tags, state) => { /* no-op */}) : shortCallback;
	}
	else if (measurementCallback is MeasurementCallback<long> longCallback)
	{
	_longMeasurementCallback = (measurementCallback is null) ? ((instrument, measurement, tags, state) => { /* no-op */}) : longCallback;
	}
	else
	{
	throw new InvalidOperationException(SR.Format(SR.UnsupportedType, typeof(T)));
	}
	}

Noticed while looking at #68022.

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[soroshsabz]

Did you want to resolve this, in my PR? or you want to create another PR?

[KalleOlaviNiemitalo]

I don't want to create a PR. I filed this as a new issue rather than as a comment in your PR, because this seems like a bug that should be fixed even if your PR adding support for uint is rejected.

[tommcdon]

@tarekgh @noahfalk

[KalleOlaviNiemitalo]

Alternatively, it could be changed to throw ArgumentNullException, but then the parameter should not be marked as nullable.

[soroshsabz]

Alternatively, it could be changed to throw ArgumentNullException, but then the parameter should not be marked as nullable.

@KalleOlaviNiemitalo I think this decision break API, so we have to handle nullability

[soroshsabz]

@tommcdon If you agree that this issue is bug, So I think, we have to resolve this for .NET 6

Are you agree with me?

[tarekgh]

I am going to fix this as originally suggested by @KalleOlaviNiemitalo in the description. The fix will be in .NET 7.0. I don't think we need to port this in .NET 6.0 as the workaround for users is they can check null in their code before calling. The users can recycle the listener too if needed.

[soroshsabz]

@tarekgh I add PR for this issue, please see and review it #68071 instead of create another one

[soroshsabz]

@tarekgh I wrote my comment on the issue, is there any wrong?

[tarekgh]

I wrote my comment on the issue, is there any wrong?

Sorry, I commented by mistake here 😄 Thanks for submitting the PR. I have added a comment on the PR.

[soroshsabz]

I am going to fix this as originally suggested by @KalleOlaviNiemitalo in the description. The fix will be in .NET 7.0. I don't think we need to port this in .NET 6.0 as the workaround for users is they can check null in their code before calling. The users can recycle the listener too if needed.

@tarekgh I think, because .NET 6 is LTS and this bug is not change any API, so we can release this for .NET 6, without any cost. and demonstrates Microsoft 's commitment to support LTS .NET

[tarekgh]

I think, because .NET 6 is LTS and this bug is not change any API, so we can release this for .NET 6, without any cost. and demonstrates Microsoft 's commitment to support LTS .NET

Usually, we do the servicing when we see any customer/user is blocked by that. I already mentioned some trivial workarounds if anyone wants to avoid this issue. Let's wait to see if more people run into this issue and then we can consider porting it. Thanks for your feedback.