search

microsoft / ApplicationInsights-Kubernetes

Enrich the telemetry data for .NET applications running inside containers that are managed by Kubernetes.
Other
135 stars 57 forks source link

Upgrade KubernetesClient to >= 10.0.16 to remove dependency on System.IO.Abstractions #332

Closed anhaehne closed 1 year ago

anhaehne commented 1 year ago

With the release of KubernetesClient 10.0.16 the package removed the depency on System.IO.Abstractions. System.IO.Abstractions has recently undergone some heavy changes which includes moving the code to a different package under the same namespace. More details in my issue in KubernetesClient Update System.IO.Abstraction to version >= 18.0.0 #1136

Without removing this dependency, all projects referencing this package can't updated to the latest System.IO.Abstractions package without a workaround.

xiaomi7732 commented 1 year ago

Hi @anhaehne, thanks for filing the issue. Let me take a look.

xiaomi7732 commented 1 year ago

Hey @anhaehne, I did some investigation, and here are things I am considering bumping K8s.Client up to 10.0.16:

In my current understanding, the biggest concern is bumping up the target framework. In theory .NET Core 3.x is out of support, in reality though, I think there's going to be a while before everyone moves their projects up to 6.x+.

A tech solution to this is to ship two versions of ApplicationInsights.Kubernetes, for example

And then, we will have the download count on NuGet to tell, the rate of people on .NET 6 vs older.

Does anyone have any other inputs, or thoughts?

// Tagging @karolz-ms @JacobBovee for awareness.

xiaomi7732 commented 1 year ago

By more investigation, another possibility is to make kubernetes client a submodule, reference the sources + K8s.Basic package since K8s.Basic is still targeting .NET Standard 2.1, then we don't need to branch out the version of our package. The problem is then, when K8s.Basic bump up to .NET 6, we then will still have to choose a side.

xiaomi7732 commented 1 year ago

Side note: Another possible approach is the multi-targeting package.

Advantage: The user doesn't have to choose from different versions of the package manually. Disadvantage: It makes the code complex - to have different branches for different versions of .NET Fx.

xiaomi7732 commented 1 year ago

IMHO, there are 2 technical proposals that are practical, and they each have pros and cons:

Approaches Pros Cons
Start 6.x, targeting only .NET 6 +
  • cleaning implementation, easy debugging
  • Download number reveals how many users moved to .NET 6 +
 User has to understand which version to pick
Multi-targeting 6.x and netstd 2.1 (.NET Core 3.1) Users do not need to pick which version to use explicitly
  • Less motivation to upgrade
  • No telemetry to tell how many users moved on.
  • More convoluted code to support different targets.

I am weighing the user experience over the developer overheads, thus PR #333. Please let me know if you disagree, and I am open to having discussions & withdrawing my opinion.

Tagging @karolz-ms, who raised the question of dropping support for .NET 3.x during the code review offline.

karolz-ms commented 1 year ago

My main concern with people staying with .NET Core 3.1 is that being out of support means no bug fixes, including no security fixes. So it is not a good place to be by any means.

This does not necessarily mean we should not support multi-targeting. But I have serious doubts if we should continue supporting .NET 3.1.

xiaomi7732 commented 1 year ago

@karolz-ms, I agree with you that we should stop supporting .NET 3.x. I am a bit hesitant to say it is the best time right now - 3.x was out of support at end of the year last year. I would be more comfortable dropping it in, say, 3 months. Of course, that also doesn't mean multi-targeting is the only solution.

I am debating myself now:

  1. There's always that project that can't be bumped up to the latest/greatest framework and shall we support it?
  2. Is multi-targeting really the best way to take it out? Especially, considering the feature stableness of this package.
anhaehne commented 1 year ago

There is also the possibility to keep a 3.x branch and keep that as up to date as possible and make 3.x the last version to support netstd 2.1. The main branch could then target .net 6 and move to version 4.x.

After a while, depending on the downloads of the 3.x packages, the netstandard 2.1 version could be dropped or further investments could be made into properly supporting netstandard 2.1.

xiaomi7732 commented 1 year ago

@anhaehne Yes, that also makes sense. Do you prefer that? It is actually easier. I propose we bump the version up to 6.0 to try to align with the lowest .NET version supported, shouldn't have a huge impact.

anhaehne commented 1 year ago

For me all these solution are acceptable as all our application are on .net 7 already. But i do understand that there are projects out there that are stuck on older version. So whatever is best for you.

xiaomi7732 commented 1 year ago

Here I conducted a small, probably biased poll on Twitter for which .NET runtime is used: image Some quick reads: 956 views, 135 votes, voting rate: 14.1%. 46.7% + 35.6% (same base) = 82.3% has already been on .NET 6 +; 6.7% is still on .NET Core 3.1.

Considering the benefits of seeing download numbers, the cleaning implementations, and also the existing versions won't be withdrawn immediately, I will submit another PR to have a 6.0 package for .NET 6 users, and have the 3.x packages for backward compatibility for a while.

Multi-targeting could still be introduced as a backup in the future if it turned out there'd be huge complaints.

Thank you @anhaehne, @karolz-ms for the discussion. Way to go, team!

xiaomi7732 commented 1 year ago

Hi @anhaehne, please find the new package here: https://www.nuget.org/packages/Microsoft.ApplicationInsights.Kubernetes/6.0.0-beta1 And let us know if this works for you.

Once verified, I will be able to push this into a stable release.

xiaomi7732 commented 1 year ago

Tagging another PR addressing re-targeting for the hosting startup project: #335

anhaehne commented 1 year ago

I've been running the pre release package for one of our applications in prod for a couple of days and everything looks fine so far.

Since the references to the nuget packages are gone, this issue is also resolved.

xiaomi7732 commented 1 year ago

Terrific. I'll prepare a stable release for it.

xiaomi7732 commented 1 year ago

Here's the 6.0.0 package: https://www.nuget.org/packages/Microsoft.ApplicationInsights.Kubernetes/6.0.0 It is basically a re-pack of 6.0.0-beta1.