Remove target frameworks net452;net46; because they are no longer sup…

antymon4o commented 8 months ago

…ported #2850

use frameworkreference for Microsoft.AspNetCore.App instead of icrosoft.AspNetCore.* nuget packages, because they are now deprecated and end of life.

Fix Issue #2850.

Changes

Remove target frameworks net452;net46; and netcoreapp3.1; because they are no longer supported. Affected projects are: DependencyCollector, TelemetryChannel, Perf, WindowsServer, Microsoft.ApplicationInsights. Use frameworkreference for Microsoft.AspNetCore.App instead of icrosoft.AspNetCore.* nuget packages, because they are now deprecated and end of life. Microsoft.ApplicationInsights.AspNetCore targets net6 to enable framework reference to Microsoft.AspNetCore.App.

Checklist

[X] I ran Unit Tests locally.
[X] CHANGELOG.md updated with one line description of the fix, and a link to the original issue if available.

The PR will trigger build, unit tests, and functional tests automatically. Please follow these instructions to build and test locally.

Notes for authors:

FxCop and other analyzers will fail the build. To see these errors yourself, compile localy using the Release configuration.

antymon4o commented 8 months ago

Thank you for you comment, @TimothyMothra. I perfectly understand that dropping support for specific framework version is not an easy decision to make. In the company I work, we have been using .NET Framework since version 1.1 for the systems we are developing. And as you can imagine, we have tons of legacy code. This is pretty normal for enterprise software that is evolving for decades. During the years we have been upgrading to the proper supported version of .NET Framework and .NET Core. But there is a difference between having legacy code and running it on unsupported platform. The former is normal, but the latter is unacceptable if only for security and compliance reasons. Of course, all the versions of ApplicationInsights packages up to 2.22 will always support net452 and net46. And thus if any legacy systems need them, they can use them. As we cannot tolerate dragging dependencies of unsupported and legacy frameworks and packages, the need is for a version of ApplicationInsights that is dependent on current, supported packages and frameworks. Given that there is no code change, and thus no behavior change in the library, what is the problem to release this new version? Will it be more acceptable if the new version of ApplicationInsights is set to 3.0 or even 6.0 to indicate major change in supported frameworks?

MichaCo commented 8 months ago

Although I would love to see this change, we have a requirement to continue supporting these frameworks for same legacy systems.

@TimothyMothra I'd suggest a major version release across the board (3.0).

There is a huge difference between AspNetCore 2.x/3.x and the latest versions under NET6/8 in terms of what nugets to reference vs using Framework dependencies...

Everything prior NET6 is long out of LTS anyways, so, why not just clean up the mess and publish a clean version for the new .NET 6/8 versions?

antymon4o commented 8 months ago

In Microsoft.ApplicationInsights project, there are a lot of comments like:

// .NET 4.5.2 have a custom implementation of RichPayloadEventSource

And in RichPayloadEventSource.cs there is a conditional compilation #if !NET452.

The question is: Is this implementation specific to NET452 only or is it specific to all framework versions in general?

If this implementation should be used in all framework versions, then this condition should be changed.

pharring commented 8 months ago

@antymon4o I believe the conditional in RichPayloadEventSource.cs is correct. That's because support for anonymous types in EventSource events was introduced in .NET 4.6. There's a comment starting on line 393 of that file explaining it.

antymon4o commented 8 months ago

@TimothyMothra can you please have another look at the PR and tell if the support for the deprecated and LTS out frameworks is critical. Is there any chance the proposed changes to be merged in a new major version 3.0 of ApplicationInsigts?

cijothomas commented 8 months ago

Is there any chance the proposed changes to be merged in a new major version 3.0 of ApplicationInsigts?

Sorry, there are no plans for a 3.0 Application Insights! However, dropping target framework can be done without major version bump! I am not suggesting we merge this PR and do normal release - I'll defer to @TimothyMothra to decide if this is acceptable or not.

It maybe worth exploring/fixing Microsoft.ApplicationInsights.AspNetCore package alone to fix the original issue described in https://github.com/microsoft/ApplicationInsights-dotnet/issues/2850, more precisely raised in https://github.com/microsoft/ApplicationInsights-dotnet/issues/2811

antymon4o commented 8 months ago

It is possible to fix only ApplicationInsights.AspNetCore package, but it would be a somewhat partial fix. Actually, the fix for this package is part of this PR. If it is useful I can post only it as a part of a new PR. I just need to know if it is definite that this PR is rejected? Who can confirm this and the accepted/approved approach to fix the issues?

antymon4o commented 8 months ago

I have created e new PR #2860 with only the changes to Microsoft.ApplicationInsights.AspNetCore package and linked it to #2811.

microsoft / ApplicationInsights-dotnet