Fix | Improve error handling when creating performance counters

[edwardneal]

Description

This may need to be backported to 6.1.

#3251 refactored DbConnectionPoolCounters (a netfx-specific class) into a platform-independent SqlClientMetrics class. While the performance counter creation logic remained the same, I introduced a bug in the exception handling.

SqlClient/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionPoolCounters.netfx.cs

Lines 104 to 122 in c12134a

	try
	{
	if (!ADP.IsEmpty(categoryName) && !ADP.IsEmpty(instanceName))
	{
	PerformanceCounter instance = new PerformanceCounter();
	instance.CategoryName = categoryName;
	instance.CounterName = counterName;
	instance.InstanceName = instanceName;
	instance.InstanceLifetime = PerformanceCounterInstanceLifetime.Process;
	instance.ReadOnly = false;
	instance.RawValue = 0; // make sure we start out at zero
	_instance = instance;
	}
	}
	catch (InvalidOperationException e)
	{
	ADP.TraceExceptionWithoutRethrow(e);
	// TODO: generate Application EventLog entry about inability to find perf counter
	}

In the original logic, if an InvalidOperationException was thrown then the exception handler ran before assigning a value to the class' performance counter. In the refactored logic, the counter was returned anyway.

The net effect of this is that if a performance counter failed to initialise (such as if the ".NET Data Provider for SqlServer" provider wasn't present) then it'd be returned in an uninitialised state. The next attempt to increment the counter would try to initialise it again, failing again.

Issues

Fixes #3595.

Testing

I don't think there's a way to add a non-intrusive automatic test, but a manual test works properly.

To break this, I ran:
unlodctr ".NET Data Provider for SqlServer"
main currently fails with an InvalidOperationException, and it's resolved by running:
lodctr "C:\Windows\INF\.NET Data Provider for SqlServer\_dataperfcounters_shared12_neutral.ini"

After this fix, the performance counters now silently fail (and tracing records the error while initializing.)

[paulmedynski]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[paulmedynski]

Change looks good. Is there a way to alter one or more of the existing integration tests to remove the counter support before running these tests? Would that have caught the regression?

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 58.45%. Comparing base (2655083) to head (3664525).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...oft/Data/SqlClient/Diagnostics/SqlClientMetrics.cs	66.66%	1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (2655083) and HEAD (3664525). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (2655083)	HEAD (3664525)
addons	1	0

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3623      +/-   ##
==========================================
- Coverage   67.94%   58.45%   -9.50%     
==========================================
  Files         276      270       -6     
  Lines       61641    61324     -317     
==========================================
- Hits        41885    35847    -6038     
- Misses      19756    25477    +5721     

Flag	Coverage Δ
addons	?
netcore	62.46% <ø> (-7.42%)	⬇️
netfx	61.30% <66.66%> (-8.71%)	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[edwardneal]

I gave some thought to writing a test, but I think it'd be pretty fragile. In theory we could have a test which manually runs unlodctr, tries to instantiate a connection, then runs lodctr. This would be a system-wide change though - any tests running in parallel on the machine would be impacted, so I think it'd risk generating test failures elsewhere if there are multiple CI runs operating (or multiple CI legs running on the same machine.)

[paulmedynski]

Fair enough, if you tested manually then I'm happy.

I did happen to just discover that all Manual Tests are being run serially, so I guess such a test would work today without perturbing other tests :) I agree that it's probably not worth it currently though.