AspNetCore.Diagnostics.HealthChecks
AspNetCore.Diagnostics.HealthChecks copied to clipboard
Fix Microsoft.Bcl.AsyncInterfaces issue
fixes #1975
Actually this fix should be done for all packages. The core issue is Microsoft.Extensions.DependencyInjection
dependency.
TODO:
- Add/ensure net6 tfm for all packages
- Use
Microsoft.Extensions.DependencyInjection
v6 for net6 tfm and v7 for any other tfm.
Rel: #1720
ping @NielsPilgaard
#1720 have fixed this issue in wrong way, alas.
In other words net6-targeted app SHOULD NOT use Microsoft.Extensions.DependencyInjection
> v6.
Would you like a hand with this PR? :)
I would like to hear considerations. I'm going to change all deps.
Where is the dependency on Microsoft.Extensions.DependencyInjection
coming from?
Many CI failed for net6.0 with error System.EntryPointNotFoundException : Entry point was not found.
after I removed
<PackageReference Include="Microsoft.Bcl.AsyncInterfaces" Version="7.0.0" Condition="'$(TargetFramework)' != 'net7.0'" />
line from Directory.Build.props
.
I narrowed it down to --collect "XPlat Code Coverage"
argument.
dotnet test ./test/HealthChecks.Gremlin.Tests/HealthChecks.Gremlin.Tests.csproj -f net6.0
works
dotnet test ./test/HealthChecks.Gremlin.Tests/HealthChecks.Gremlin.Tests.csproj --collect "XPlat Code Coverage" -f net6.0
fails.
If I return <PackageReference Include="Microsoft.Bcl.AsyncInterfaces" Version="7.0.0" Condition="'$(TargetFramework)' != 'net7.0'" />
back everything works again but I don't understand why this package is still an issue. For net6 I aligned package versions to <7.
Need a bit assistance here.
@NielsPilgaard From our code only 1 place (abstractions though)
@NielsPilgaard Ah, I misunderstood you. It came from Microsoft.Extensions.Diagnostics.HealthChecks
that is used by each HC package.
Looks like an issue with coverlet.collector, I would like to deal with it before making further changes here.
@NielsPilgaard Ah, I misunderstood you. It came from
Microsoft.Extensions.Diagnostics.HealthChecks
that is used by each HC package.
I only see Microsoft.Extensions.DependencyInjection.Abstractions
as a dependency to Microsoft.Extensions.Diagnostics.HealthChecks
?
How do we know Microsoft.Extensions.DependencyInjection
is the underlying issue here?
Perhaps a stupid question, but do we really need code coverage?
I only see Microsoft.Extensions.DependencyInjection.Abstractions as a dependency to Microsoft.Extensions.Diagnostics.HealthChecks?
Microsoft.Extensions.Diagnostics.HealthChecks -> Microsoft.Extensions.Hosting.Abstractions -> Microsoft.Bcl.AsyncInterfaces
Perhaps a stupid question, but do we really need code coverage?
Code coverage is a widely used feature that helps to observe the level of testability.
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
73abc7a
) 64.11% compared to head (30e1183
) 64.86%.
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@ Coverage Diff @@
## master #1980 +/- ##
==========================================
+ Coverage 64.11% 64.86% +0.75%
==========================================
Files 248 264 +16
Lines 8359 8583 +224
Branches 586 613 +27
==========================================
+ Hits 5359 5567 +208
- Misses 2852 2860 +8
- Partials 148 156 +8
Flag | Coverage Δ | |
---|---|---|
ApplicationStatus | 26.66% <ø> (ø) |
|
ArangoDb | 26.50% <ø> (ø) |
|
Aws.S3 | 14.28% <ø> (ø) |
|
Aws.SecretsManager | 14.54% <ø> (ø) |
|
Aws.Sns | 14.77% <ø> (ø) |
|
Aws.Sqs | 15.28% <ø> (ø) |
|
Aws.SystemsManager | 14.54% <ø> (ø) |
|
Azure.IoTHub | 12.82% <ø> (ø) |
|
AzureApplicationInsights | 14.70% <ø> (ø) |
|
AzureBlobStorage | 25.17% <ø> (?) |
|
AzureDigitalTwin | 35.76% <ø> (ø) |
|
AzureEventHubs | 16.54% <ø> (?) |
|
AzureFileStorage | 25.17% <ø> (?) |
|
AzureKeyVault | 29.24% <ø> (ø) |
|
AzureQueueStorage | 25.17% <ø> (?) |
|
AzureSearch | 16.10% <ø> (ø) |
|
AzureServiceBus | 71.34% <ø> (ø) |
|
AzureTableStorage | 26.66% <ø> (?) |
|
Consul | 24.32% <ø> (+1.74%) |
:arrow_up: |
CosmosDb | 28.10% <ø> (ø) |
|
Dapr | 14.50% <ø> (?) |
|
DocumentDb | 14.64% <ø> (ø) |
|
DynamoDb | 12.12% <ø> (ø) |
|
Elasticsearch | 39.49% <ø> (ø) |
|
EventStore | 62.08% <ø> (ø) |
|
EventStore.gRPC | 25.51% <ø> (ø) |
|
Gcp.CloudFirestore | 12.10% <ø> (ø) |
|
Gremlin | 25.00% <ø> (+1.92%) |
:arrow_up: |
Hangfire | 10.97% <ø> (ø) |
|
IbmMQ | 30.76% <ø> (ø) |
|
InfluxDB | 15.00% <ø> (ø) |
|
Kafka | 22.03% <ø> (ø) |
|
Kubernetes | 41.54% <ø> (ø) |
|
MongoDb | 31.77% <ø> (ø) |
|
MySql | 31.63% <ø> (ø) |
|
Nats | 72.77% <ø> (+2.91%) |
:arrow_up: |
Npgsql | 42.71% <ø> (+15.61%) |
:arrow_up: |
OpenIdConnectServer | 39.28% <ø> (ø) |
|
Oracle | 60.60% <ø> (ø) |
|
Prometheus.Metrics | 29.80% <ø> (ø) |
|
Publisher.ApplicationInsights | 14.76% <ø> (ø) |
|
Publisher.CloudWatch | 19.02% <ø> (ø) |
|
Publisher.Datadog | 15.00% <ø> (ø) |
|
Publisher.Prometheus | 18.75% <ø> (ø) |
|
Publisher.Seq | 40.30% <ø> (ø) |
|
RabbitMQ | 47.67% <ø> (ø) |
|
RavenDb | 70.74% <ø> (ø) |
|
Redis | 65.71% <ø> (ø) |
|
SendGrid | 11.92% <ø> (ø) |
|
SignalR | 24.22% <ø> (ø) |
|
SqlServer | 28.57% <ø> (ø) |
|
Sqlite | 26.38% <ø> (ø) |
|
System | 42.56% <ø> (ø) |
|
UI | 65.68% <ø> (-0.17%) |
:arrow_down: |
Uris | 0.00% <ø> (ø) |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@NielsPilgaard Hi! A lot of time has passed since I was doing this. Now I am gradually returning to the support of this project. I resolved conflicts and see that now the tests are successful, although I have not changed coverlet version. Perhaps, NET8 SDK is the case. No matter how it was, we are returning to the initial question about which versions of Microsoft.Extensions.DependencyInjection
to use for different TFMs. Bug with Microsoft.Bcl.AsyncInterfaces
is rather annoying and I would like to make changes in all projects to fix it once and forever.
This PR is an example of what I expect to do - retarget packages to use different Microsoft.Extensions.DependencyInjection
versions so each package on netX TFM uses Microsoft.Extensions.DependencyInjection
vX. In case of netstandard2.0/2.1 TFMs I think it's worth to use the latest Microsoft.Extensions.DependencyInjection
, i.e. 8, but I'm not sure.
ping @adamsitnik @unaizorrilla for advice.
This PR is an example of what I expect to do - retarget packages to use different Microsoft.Extensions.DependencyInjection versions so each package on netX TFM uses Microsoft.Extensions.DependencyInjection vX. In case of netstandard2.0/2.1 TFMs I think it's worth to use the latest Microsoft.Extensions.DependencyInjection, i.e. 8, but I'm not sure.
Good plan, I agree. Also, using v8 for netstandard2.0/2.1 seems correct, I can't see a scenario where that would break anything.
Oh, I will need to check about 70 projects.
Oh, I will need to check about 70 projects.
How can I help? If you want help that is 😊
If you want you may post a PR into my branch adding all that stuff in csproj files. I will not deal with this PR today or tomorrow but can review/merge.
While working on that PR I saw that Microsoft.Bcl.AsyncInterfaces
has been included in Microsoft.Extensions.Diagnostics.HealthChecks
since v8.0.0, when targetting anything other than net8.0
- Since that's the case we should just be able to update to v8.0.0 across the board, and the bug should be resolved, no?
I'm not sure. Yes, it is included but it has version 8, so net6/net7 apps end up with v8 instead of v6. There are 2 questions - backward compatibility and consistency across platforms. I tend to be consistent and to not use v8 in net6/7 TFMs.
Personally I prefer being able to stay on the latest version, and not have to stick with a specific one matching my current TFM.
In ideal world everyone wants this but these are cross restrictions on versions used in app which is combined of all packages in dependency tree.
So you think we should go with the approach you outlined earlier?
I think so. https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/issues/1975#issuecomment-1837925404 should be applied not only for our clients but also for us because we are the clients of underlying MS packages.
Well, actually https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/issues/1975#issuecomment-1837925404 may say that v8 versions of health checks should work ONLY for net8 and for net6 one should use previous series of packages. @adamsitnik please clarify. If so we can completely separate targeting to different maintenance branches - v6, v7, v8. It is rather serious decision and subsequent work.
I end up with 3 options for versioning strategy.
If not care too much about platform compatibility:
Option 1. Always target the latest MS packages in master
. For now 8.
If care about platform compatibility:
Option 2. Multitarget packages in master
to use v6/v7/v8 series of MS packages, so our v8 supports v8 AND v7 AND v6, etc
Option 3. Target packages in master
to use ONLY v8 series of MS packages, our v8 supports v8 (+netstandard). Target packages in v7
branch to use ONLY v7 series of MS packages, our v7 supports v7 (+netstandard). Target packages in v6
branch to use ONLY v6 series of MS packages, our v6 supports v6 (+netstandard).
Option 3 IMO looks more attractive from a client point of view. It copies MS versioning strategy for .NET packages. The drawback is that we need to backport changes between branches. This is tiring work.
Option 2 is almost where we are for now but further changes required to be fully compatible with such strategy.
In any case chosen option should be explicitly documented in README.
To reduce complexity and maintenance load I prefer option 1, but I agree with your analysis from the consumer's point of view.
I'll hold off on making a PR until a decision has been made :)
I am going to think out loud here.
- Each health check package depends on
Microsoft.Extensions.DependencyInjection
(and maybe some other Microsoft.Extensions packages) and the client library package(s). The first follow .NET versioning scheme, the latter sometimes do (example: Npgsql), but most of them don't. -
Microsoft.Extension
packages should not introduce breaking API changes (old code should compile just fine), but they may drop older TFM support (for now it's very rare, but one day .NET Team is going to stop supporting .NET Standard). - Why users may want to update to latest version of Health Check packages:
a) get a bug fix for a bug in the health check package.
b) update to new .NET and stop using two different versions of
Microsoft.Extensions
packages c) get a bug fix for MS.E or client library package (the users can just add the reference to newer version in explicit way and everything is going to work fine).
If every package targets different MS.E version depending on the TFM, we don't need to backport bug fixes to older versions, but we may need to use #if
when using new features for new TFMs. It may also require changes to the README.md
files for cases when we recommend using new features like today with keyed DI to register multiple instances of the same type.
If every package targets only latest TFM, we should backport every bug fix to previous version. It's time consuming and requires extra work. But it's rare.
We could give it a try and change all the packages to do the following:
<PackageReference Include="Microsoft.Extensions.DependencyInjection" Version="$(MicrosoftExtensionsVersion)" />
And in Directory.Build.props
:
<!-- out-of-band packages that are not included in NetCoreApp have TFM-specific versions -->
<Choose>
<When Condition="'$(TargetFramework)' == 'net6.0'">
<PropertyGroup>
<MicrosoftExtensionsVersion>6.0.1</MicrosoftExtensionsVersion>
</PropertyGroup>
</When>
<When Condition="'$(TargetFramework)' == 'net7.0'">
<PropertyGroup>
<MicrosoftExtensionsVersion>7.0.0</MicrosoftExtensionsVersion>
</PropertyGroup>
</When>
<When Condition="'$(TargetFramework)' == 'net8.0'">
<PropertyGroup>
<MicrosoftExtensionsVersion>8.0.0</MicrosoftExtensionsVersion>
</PropertyGroup>
</When>
</Choose>
But I am not sure how to handle .NET Standard TFM here. I guess it should target the latest version??
If we are lucky, the update to .NET 9 would require:
<When Condition="'$(TargetFramework)' == 'net9.0'">
<PropertyGroup>
<MicrosoftExtensionsVersion>9.0.0</MicrosoftExtensionsVersion>
</PropertyGroup>
</When>
But.. introducing such change would be a braking change for all the users who target older TFMs, updated to our 8.0.0
packages and now would update to 8.1.0
which would change the version of referenced MS.E
packages.
But I don't think that it's common and that it would cause any actual trouble.
I think that we could try to apply these changes to 10 most popular packages and see how it goes. Then based on the complexity of required changes decide whether we want to introduce such changes.