search

open-telemetry / opentelemetry-dotnet

The OpenTelemetry .NET Client
https://opentelemetry.io
Apache License 2.0
3.23k stars 764 forks source link

Remove frameworks older than net461 #2138

Closed cijothomas closed 3 years ago

cijothomas commented 3 years ago

DiagnosticSource 6.0 only supports net461 and above. (i.e it drops net452,net46 support, few months before they are officially going unsupported - https://devblogs.microsoft.com/dotnet/net-framework-4-5-2-4-6-4-6-1-will-reach-end-of-support-on-april-26-2022/) Since the API/SDK/Exporters depend on DiagnosticSource , they can now only support net461 and above.

Proposing to drop support for NET452, NET46 in the next release (1.2.0). What this means is, when we release the next minor version (1.2.0) in Nov 2021, we'll be cutting support for some frameworks (net452, net46 specifically). This might be considered a breaking-change, and warrant a major version bump.

However, I think we can (and we should) do minor version bump only, considering NET452/46 support is dropped by DS (which we have hard dependency on), and Net452/net46 will be EOL within 4 months of our planned 1.2 release.

@open-telemetry/dotnet-approvers @open-telemetry/dotnet-maintainers Please share your concerns if any with this. Otherwise, please mark approval by commenting to the issue.

joaopgrassi commented 3 years ago

This seems to be an easy one :). If you need help I could take it.

cijothomas commented 3 years ago

This seems to be an easy one :). If you need help I could take it.

Thanks!

https://github.com/open-telemetry/opentelemetry-dotnet/pull/2139 Just merged one of the projects! you can pick one and do similar. We need it to do it step by step, starting with the "leaf" ones, and finally the Otel.API :)

Please put a comment here saying which one you are taking!

joaopgrassi commented 3 years ago

OK! I'll not bite more than I can chew, so I'll pick for now these which should be fairly quick it seems:

joaopgrassi commented 3 years ago

Do we also want to remove the files on .publicApi for these targets?

cijothomas commented 3 years ago

Do we also want to remove the files on .publicApi for these targets?

unshipped, yes. shipped - lets keep it. (need to get more feedback about whether to keep it or not..)

joaopgrassi commented 3 years ago

Taking OpenTelemetry.Instrumentation.SqlClient (and test) next..

ejsmith commented 3 years ago

I argued for this multiple times and it would've made the DI and logging stuff much simpler and much more inline with .NET Core, but I'm guessing now the library is stuck with it's own abstractions for these things and all the limitations that those abstractions have.

SergeyKanzhelev commented 3 years ago

lgtm

CodeBlanch commented 3 years ago

I'm good with a minor bump!

ghost commented 3 years ago

Just a heads up, a few of us out here support teams compiling against net452. There were a lot of build breaks introduced in the net46x time frame, and they're still trying to catch up on massive code bases.

This isn't a bad change, I get where you're coming from and honestly support this move. I just want to make sure you're aware that this will lock in some large enterprise-level projects to version 1.1.x for potentially 12+ months.

(keep in mind that net452's support window is currently longer than any netcore/net5.0 GA support window. In that sense, it's "newer" than net5.0. I can support peers moving to net452 better than I can support peers moving to net5.0, since only one of those two will be alive 12 months from now)

ejsmith commented 3 years ago

Just a heads up, a few of us out here support teams compiling against net452. There were a lot of build breaks introduced in the net46x time frame, and they're still trying to catch up on massive code bases.

This isn't a bad change, I get where you're coming from and honestly support this move. I just want to make sure you're aware that this will lock in some large enterprise-level projects to version 1.1.x for potentially 12+ months.

(keep in mind that net452's support window is currently longer than any netcore/net5.0 GA support window. In that sense, it's "newer" than net5.0. I can support peers moving to net452 better than I can support peers moving to net5.0, since only one of those two will be alive 12 months from now)

@aaronla-ms I'm very curious here. So you are telling me that they have apps running net452 that they haven't maintained enough to bring it up to a more current version of .NET, but they have made use of a brand new OpenTelemetry client that just went 1.0 not that long ago?

cijothomas commented 3 years ago

I just want to make sure you're aware that this will lock in some large enterprise-level projects to version 1.1.x for potentially 12+ months.

The 1.2.0 version, planned to be released by Nov 2021, is where we'll be cutting net452. As described in the milestones (https://github.com/open-telemetry/opentelemetry-dotnet/milestones), there are only 2 things which are part of 1.2.0 - .NET 6 support, Metrics support. Both of this feature should not be a concern for projects using net452, as they shouldn't care about .NET 6 support (must be obvious!), and Metrics (as DiagnosticSource v6.0, the one that has metrics API, don't support net452 anyway.)

ghost commented 3 years ago

Thanks for the update! Sounds like we'll dual target then, 1.2 for net462+ and 1.1 for net452

reyang commented 3 years ago

@aaronla-ms I'm very curious here. So you are telling me that they have apps running net452 that they haven't maintained enough to bring it up to a more current version of .NET, but they have made use of a brand new OpenTelemetry client that just went 1.0 not that long ago?

I guess when the owner of a large codebase decided to upgrade, one thing they would start with is to add some telemetry so they can keep things monitored while making changes. For example, when folks upgrade from netcore2.1 to netcore3.0, many of them rely on telemetry SDKs for troubleshooting the upgrade (whether logical issue or perf issue).

ghost commented 3 years ago

@aaronla-ms I'm very curious here. So you are telling me that they have apps running net452 that they haven't maintained enough to bring it up to a more current version of .NET, but they have made use of a brand new OpenTelemetry client that just went 1.0 not that long ago?

I guess when the owner of a large codebase decided to upgrade, one thing they would start with is to add some telemetry so they can keep things monitored while making changes. For example, when folks upgrade from netcore2.1 to netcore3.0, many of them rely on telemetry SDKs for troubleshooting the upgrade (whether logical issue or perf issue).

Reyang is spot on. Sometimes software has to keep working, and we can't just halt everything for a rewrite.

In this case, different teams own the telemetry vs the actual service. I'm working on the telemetry, and we're revamping that from our internal custom telemetry to shift over to open telemetry, while minimizing disruption to the actual service.

In contrast, the service in question is a behemoth that serves massive amounts of traffic daily, and has been doing so for over a decade at this point. Remember, the bulk of it was written when net45 was still relatively new. And remember that the service is actually running on net472 or net48 runtime at this point. It just compiles against net45. There are build breaking changes they have to work through to port forward to net462 build targets.

@cijothomas wrote:

Both of this feature should not be a concern for projects using net452, as they shouldn't care about .NET 6 support (must be obvious!),

Actually, our telemetry components do need to support internal teams running the gamut from net45 thru net48, netcore21, netcore31, and net6.0 (no, we don't support net5.0 -- we only support LKG versions). Hopefully that makes some sense, and gives you a better picture of what I'm looking at.

It looks like the solution for us is to consume 1.1 in our net45 builds, and 1.2 in our net462 and higher builds, using msbuild conditionals on targetframework. Thanks!

ejsmith commented 3 years ago

@aaronla-ms MS is pretty good about not breaking things if you are just upgrading the runtime framework version and not updating the libraries like MVC or whatever else is being used. Is it not possible to easily upgrade just the runtime framework version without much if any work?

ghost commented 3 years ago

@ejsmith You've had better luck than I, I guess. I in this case at least, it is a bunch of work. There are breaking changes in a number of referenced assemblies when moving net45 to net462, as well as changes in behavior of msbuild around selecting reference assemblies for compilation when targeting net462. There's a team on it, but it'll take some time for them to sort it out.

While the code runs fine on net462+, it doesn't build when targeting net462.

ejsmith commented 3 years ago

@aaronla-ms interesting. Yeah, I've not had the same problems I guess, but I tend to try and keep up to date with the stable versions in my apps. The problem is that there are massive architectural decisions being made by having to support really old versions of .NET in this brand new library. I personally feel like some bad compromises are being made to support those old frameworks and now it seems like the team just raised the minimum to versions of the framework that would've greatly changed how things like DI and logging would be done in this library. It's always a balance and to me it just seems like this library is supposed to be the future of .NET telemetry and it should embrace things like the DI and logging abstractions from MS.