Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Target Microsoft.ApplicationInsights.AspNetCore to netcoreapp3.1 and remove package references to deprecated nugets Microsoft.AspNetCore.* #2860

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Target Microsoft.ApplicationInsights.AspNetCore to netcoreapp3.1 and remove package references to deprecated nugets Microsoft.AspNetCore.* #2860

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
abdf428
Target Microsoft.ApplicationInsights.AspNetCore to netcoreapp3.1
antymon4o Mar 28, 2024
75bbb04
In ApplicationFolderProvider.IsWindowsOperatingSystem replace conditi…
antymon4o Mar 28, 2024
a56518b
In Microsoft.ApplicationInsights.WorkerService add target framework n…
antymon4o Mar 28, 2024
84c2b03
Add comments in CHANGELOG.md
antymon4o Mar 28, 2024
35bdffd
Replace usage of obsolete IHostingEnvironment with IHostEnvironment
antymon4o Mar 29, 2024
c9c4e37
In ApplicationInsights.AspNetCore.Tests use IHostEnvironment instead …
antymon4o Mar 30, 2024
bf9b9b8
In Microsoft.ApplicationInsights.AspNetCore add target for net462
antymon4o Apr 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .props/_GlobalStaticVersion.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
Update for every public release.
-->
<SemanticVersionMajor>2</SemanticVersionMajor>
<SemanticVersionMinor>22</SemanticVersionMinor> <!-- If changing the Minor version, also update the Date value. -->
<SemanticVersionMinor>23</SemanticVersionMinor> <!-- If changing the Minor version, also update the Date value. -->
<SemanticVersionPatch>0</SemanticVersionPatch>
<PreReleaseMilestone></PreReleaseMilestone> <!--Valid values: beta1, beta2, EMPTY for stable -->
<PreReleaseMilestone Condition="'$(Redfield)' == 'True'">redfield</PreReleaseMilestone>
Expand All @@ -23,7 +23,7 @@
as it will restart file versions so 2.4.0-beta1 may have higher
file version (like 2.4.0.2222) than 2.4.0-beta2 (like 2.4.0.1111)
-->
<SemanticVersionDate>2022-07-20</SemanticVersionDate>
<SemanticVersionDate>2024-03-28</SemanticVersionDate>

<!--
BuildNumber uniquely identifies all builds (The max allowed value is UInt16.MaxValue = 65535).
Expand Down
2 changes: 1 addition & 1 deletion BASE/src/ServerTelemetryChannel/Implementation/ApplicationFolderProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ public IPlatformFolder GetApplicationFolder()

internal static bool IsWindowsOperatingSystem()
{
#if NET452
#if NETFRAMEWORK
return true;
#else
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
Expand Down
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,12 @@
## VNext
- [Populate required field Message with "n/a" if it is empty](https://github.com/microsoft/ApplicationInsights-dotnet/issues/1066)

## Version 2.23.0
- [Microsoft.ApplicationInsights.AspNetCore used deprecated NuGet packages](https://github.com/microsoft/ApplicationInsights-dotnet/issues/2811)
- Target Microsoft.ApplicationInsights.AspNetCore to netcoreapp3.1
- Remove package references to nugets Microsoft.AspNetCore.*
- For AspNetCore use framework reference to Microsoft.AspNetCore.App

## Version 2.22.0
- no changes since beta.

Expand Down
28 changes: 3 additions & 25 deletions .../Microsoft.ApplicationInsights.AspNetCore/Microsoft.ApplicationInsights.AspNetCore.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@

<PropertyGroup>
<AssemblyName>Microsoft.ApplicationInsights.AspNetCore</AssemblyName>
<TargetFrameworks>netstandard2.0</TargetFrameworks>
<TargetFrameworks>netcoreapp3.1</TargetFrameworks>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't removing netstandard2.0 lose support for asp.net core 2.1 a on .NET framework?

https://dotnet.microsoft.com/en-us/platform/support/policy/dotnet-core#servicing
image

or is that already unsupported?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ASP.NET Core 2.1 is the de facto version that is already deprecated and no longer supported.
Removing dependency to deprecated ASP.NET Core 2.1 nuget packages is the goal of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its a bit more complex... ASP.NET Core 2.1 is deprecated, however, ASP.NET Core 2.1 on .NET Framework is not...
I believe this is still a supported version in this repo, so won't be possible to remove it.

Also, given almost all of new investments are occurring only in OpenTelemetry, it may not be desired to spend much time on this repo. Of course, if there are security issues, it should be promptly addressed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct. ASP.NET Core 2.1.x is supported on .NET Framework.
Given that MS split the support for ASP.NET Core on .NET Core and .NET Framework, maybe a better approach is to also split the targets of AI.AspNetCore - for net462 and netcoreapp3.1:

  • For net462, AI.AspNetCore will use nuget packages for ASP.NET Core 2.1.x
  • For netcoreapp3.1, AI.AspNetCore will use framework reference to Microsoft.AspNetCore.App.

This way AI.AspNetCore will continue to support ASP.NET Core 2.1.x on .NET Framework through the nuget package.

How does this sound to you?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might be correct. Unfortunately, I do not have enough cycles to think through all the implications of that.

@TimothyMothra can make a final call if we want to proceed with this.

@antymon4o Apologies in advance, if we chose to not proceed with this. Almost all investments are occurring in OpenTelemetry based solutions, given it is the future for instrumentation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cijothomas thank you for you comments!
I understand that the focus and the future is in the OpenTelemetry.
But since the changes in this PR are only on targeted frameworks and nuget packages, and there is no behavior change, I can not understand what resources or investments are required to review and complete this PR. The work is completed. It is not needed for someone else to do/code something more.

Is it possible at least for the PR checks to be run just to see if the builds and the test are passing on GitHub too?


<TargetFrameworks Condition=" '$(OS)' != 'Windows_NT' ">netstandard2.0</TargetFrameworks>
<TargetFrameworks Condition=" '$(OS)' != 'Windows_NT' ">netcoreapp3.1</TargetFrameworks>
<DefineConstants>$(DefineConstants);AI_ASPNETCORE_WEB;</DefineConstants>
</PropertyGroup>

Expand All @@ -32,32 +32,10 @@
<ProjectReference Include="..\..\..\WEB\Src\WindowsServer\WindowsServer\WindowsServer.csproj" />
<ProjectReference Include="..\..\..\WEB\Src\EventCounterCollector\EventCounterCollector\EventCounterCollector.csproj" />
<ProjectReference Include="..\..\..\LOGGING\src\ILogger\ILogger.csproj" />

<PackageReference Include="Microsoft.AspNetCore.Hosting" Version="2.1.1" />
<PackageReference Include="Microsoft.Extensions.Configuration.Json" Version="3.1.0" />
</ItemGroup>

<ItemGroup>
<!--
Microsoft.AspNetCore.Http has a vulnerability https://msrc.microsoft.com/update-guide/vulnerability/CVE-2020-1045
System.Text.Encodings.Web has a vulnerability https://msrc.microsoft.com/update-guide/vulnerability/CVE-2021-26701

These are both implicit dependencies from Microsoft.AspNetCore.Hosting.
(Microsoft.AspNetCore.Hosting > Microsoft.AspNetCore.Http)
(Microsoft.AspNetCore.Hosting > Microsoft.AspNetCore.Hosting.Abstractions > Microsoft.AspNetCore.Http.Abstractions > System.Text.Encodings.Web)
-->

<!--
Taking a dependency on Microsoft.AspNetCore.Hosting v2.2.0 would resolve this issue, but would also break support for NetCore v2.1.
Instead I'm taking a direct dependency on the fixed version Microsoft.AspNetCore.Http.
We can remove this when NetCore v2.1 reaches EOL on August 21, 2021.
-->
<PackageReference Include="Microsoft.AspNetCore.Http" Version="2.1.22" />

<!--
We must take a temporary dependency on this newer version until Microsoft.AspNetCore.Hosting updates their dependencies.
-->
<PackageReference Include="System.Text.Encodings.Web" Version="4.7.2" />
<FrameworkReference Include="Microsoft.AspNetCore.App" />
</ItemGroup>

<ItemGroup>
Expand Down
2 changes: 1 addition & 1 deletion ...soft.ApplicationInsights.WorkerService/Microsoft.ApplicationInsights.WorkerService.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

<PropertyGroup>
<AssemblyName>Microsoft.ApplicationInsights.WorkerService</AssemblyName>
<TargetFrameworks>netstandard2.0</TargetFrameworks>
<TargetFrameworks>net462;netstandard2.0</TargetFrameworks>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this, is unrelated?

Copy link
Author

@antymon4o antymon4o Mar 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that there isn't, but after the changes to AI.AspNetCore project some tests for AI.WorkerService.Tests for framework 4.* failed.
There isn't direct dependency between AI.AspNetCore and AI.WorkerService, But...
There was runtime error when executing the tests with this exception:

System.IO.FileLoadException: 'Could not load file or assembly 'System.Security.Principal.Windows, Version=4.1.1.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)' at Microsoft.ApplicationInsights.WindowsServer.TelemetryChannel.Shared.Implementation.WindowsIdentityProvider.Microsoft.ApplicationInsights.WindowsServer.TelemetryChannel.Shared.Implementation.IIdentityProvider.GetName() in X:\github\ApplicationInsights-dotnet\BASE\src\ServerTelemetryChannel\Managed\Shared\Implementation\WindowsIdentityProvider.cs:line 15 at Microsoft.ApplicationInsights.WindowsServer.TelemetryChannel.Implementation.ApplicationFolderProvider.CreateTelemetrySubdirectory(DirectoryInfo root) in X:\github\ApplicationInsights-dotnet\BASE\src\ServerTelemetryChannel\Implementation\ApplicationFolderProvider.cs:line 271

To fix the tests, adding net462 to AI.WorkerService was required.
And it seems logical to have this specific target for AI.WorkerService, because AI.WorkerService depends on TelemetryChannel which has two target frameworks net452, netstandard2.0 and has conditional compilation statements dependent on NET452/NETFRAMEWORK.

I cannot give an explanation why the tests were passing before. Maybe it was coincidence because this assembly System.Security.Principal.Windows was copied as a transitive dependency of Microsoft.AspNetCore.* in test output folder without directly depending, even though the code required it.

<DefineConstants>$(DefineConstants);AI_ASPNETCORE_WORKER;</DefineConstants>
</PropertyGroup>

Expand Down
21 changes: 9 additions & 12 deletions NETCORE/test/FunctionalTests.MVC.Tests/FunctionalTests.MVC.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">
<Import Project="$(PropsRoot)\Test.props" />


Expand Down Expand Up @@ -37,17 +37,6 @@
</ItemGroup>

<ItemGroup>
<PackageReference Include="Microsoft.AspNetCore" Version="2.1.1" />
<PackageReference Include="Microsoft.AspNetCore.Authentication.Cookies" Version="2.1.1" />
<PackageReference Include="Microsoft.AspNetCore.Diagnostics" Version="2.1.1" />
<PackageReference Include="Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore" Version="2.1.1" />
<PackageReference Include="Microsoft.AspNetCore.Identity" Version="2.1.39" />
<PackageReference Include="Microsoft.AspNetCore.Identity.EntityFrameworkCore" Version="2.1.1" />
<PackageReference Include="Microsoft.AspNetCore.Mvc" Version="2.1.1" />
<PackageReference Include="Microsoft.AspNetCore.Mvc.TagHelpers" Version="2.2.0" />
<PackageReference Include="Microsoft.AspNetCore.Server.IISIntegration" Version="2.2.1" />
<PackageReference Include="Microsoft.AspNetCore.Server.Kestrel" Version="2.2.0" />
<PackageReference Include="Microsoft.AspNetCore.StaticFiles" Version="2.2.0" />
<PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" Version="2.0.0" />
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.11.0" />
<PackageReference Include="xunit.runner.visualstudio" Version="2.4.3">
Expand All @@ -57,6 +46,14 @@
<PackageReference Include="xunit" Version="2.4.1" />
<DotNetCliToolReference Include="dotnet-xunit" Version="2.3.1" />
</ItemGroup>

<ItemGroup Condition=" '$(TargetFramework)' == 'netcoreapp3.1' ">
<PackageReference Include="Microsoft.AspNetCore.Mvc.Razor.RuntimeCompilation" Version="3.1.32" />
</ItemGroup>

<ItemGroup Condition=" '$(TargetFramework)' != 'netcoreapp3.1' ">
<PackageReference Include="Microsoft.AspNetCore.Mvc.Razor.RuntimeCompilation" Version="6.0.27" />
</ItemGroup>

<ItemGroup>
<DotNetCliToolReference Include="Microsoft.VisualStudio.Web.CodeGeneration.Tools" Version="2.0.0" />
Expand Down
5 changes: 4 additions & 1 deletion NETCORE/test/FunctionalTests.MVC.Tests/Startup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,10 @@ public void ConfigureServices(IServiceCollection services)

services.AddSingleton(typeof(ITelemetryChannel), new InMemoryChannel());
services.AddApplicationInsightsTelemetry(applicationInsightsOptions);
services.AddMvc();
services.AddMvcCore(options => options.EnableEndpointRouting = false);

services.AddControllersWithViews()
.AddRazorRuntimeCompilation();
}

// This method gets called by the runtime. Use this method to configure the HTTP request pipeline.
Expand Down
2 changes: 2 additions & 0 deletions NETCORE/test/FunctionalTests.Utils/TelemetryTestsBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ protected HttpResponseMessage ExecuteRequest(string requestPath, Dictionary<stri

using (HttpClient httpClient = new HttpClient(httpClientHandler, true))
{
httpClient.Timeout = TimeSpan.FromMilliseconds(TestListenerTimeoutInMs);

this.output.WriteLine($"{DateTime.Now:MM/dd/yyyy hh:mm:ss.fff tt}: Executing request: {requestPath}");
HttpRequestMessage request = new HttpRequestMessage(HttpMethod.Get, requestPath);
if (headers != null)
Expand Down
2 changes: 1 addition & 1 deletion NETCORE/test/FunctionalTests.WebApi.Tests/Controllers/DependencyController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ public async Task Get()
{
// Microsoft.com will return a redirect to a specific lang version.
// This redirect is not detected in versions older that Net6.0.
await hc.GetAsync("https://www.microsoft.com/en-us/").ContinueWith(t => { }); // ignore all errors
await hc.GetAsync("https://visualstudio.microsoft.com/msdn-platforms/").ContinueWith(t => { }); // ignore all errors
}
}
}
Expand Down
8 changes: 5 additions & 3 deletions NETCORE/test/FunctionalTests.WebApi.Tests/FunctionalTest/RequestCorrelationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ public void TestRequestWithRequestIdAndTraceParentHeader()
}

[Fact]
public void TestRequestWithRequestIdAndTraceParentHeaderWithW3CDisabled()
public void TestRequestWithRequestIdHeaderWithW3CDisabled()
{
try
{
Expand All @@ -239,8 +239,10 @@ public void TestRequestWithRequestIdAndTraceParentHeaderWithW3CDisabled()
{
// Both request id and traceparent
["Request-Id"] = "|8ee8641cbdd8dd280d239fa2121c7e4e.df07da90a5b27d93.",
["traceparent"] = "00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01",
["tracestate"] = "some=state",
// If traceparent is sent in the request, it seems that is has more priority then Request-Id
// so, do not send traceparent and tracestate if you want to use Request-Id and test Hierchical Tracing
//["traceparent"] = "00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01",
//["tracestate"] = "some=state",
["Correlation-Context"] = "k1=v1,k2=v2"
};

Expand Down
5 changes: 0 additions & 5 deletions NETCORE/test/FunctionalTests.WebApi.Tests/FunctionalTests.WebApi.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,6 @@
</ItemGroup>

<ItemGroup>
<PackageReference Include="Microsoft.AspNetCore.Mvc" Version="2.1.1" />
<PackageReference Include="Microsoft.AspNetCore.Mvc.WebApiCompatShim" Version="2.1.1" />
<PackageReference Include="Microsoft.AspNetCore.StaticFiles" Version="2.2.0" />
<PackageReference Include="Microsoft.AspNetCore.Diagnostics" Version="2.1.1" />
<PackageReference Include="Microsoft.AspNetCore.Server.Kestrel" Version="2.2.0" />
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.11.0" />
<PackageReference Include="xunit.runner.visualstudio" Version="2.4.3">
<PrivateAssets>all</PrivateAssets>
Expand Down
4 changes: 2 additions & 2 deletions NETCORE/test/FunctionalTests.WebApi.Tests/Startup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public void ConfigureServices(IServiceCollection services)
services.AddSingleton<EndpointAddress>(endpointAddress);
services.AddSingleton(typeof(ITelemetryChannel), new InMemoryChannel() { EndpointAddress = endpointAddress.ConnectionString, DeveloperMode = true });
services.AddApplicationInsightsTelemetry(InProcessServer.IKey);
services.AddMvc();
services.AddMvcCore(options => options.EnableEndpointRouting = false);
}

// This method gets called by the runtime. Use this method to configure the HTTP request pipeline.
Expand All @@ -37,7 +37,7 @@ public void Configure(IApplicationBuilder app, IHostingEnvironment env)
app.UseMvc(routes =>
{
// Add the following route for porting Web API 2 controllers.
routes.MapWebApiRoute("DefaultApi", "api/{controller}/{id?}");
routes.MapRoute("DefaultApi", "api/{controller}/{id?}");
});
}
}
Expand Down
1 change: 0 additions & 1 deletion ...nInsights.AspNetCore.Tests/Extensions/ApplicationInsightsExtensionsTests/BaseTestClass.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ namespace Microsoft.Extensions.DependencyInjection.Test
using Microsoft.ApplicationInsights.AspNetCore.Tests;
using Microsoft.ApplicationInsights.Channel;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Hosting.Internal;
using Microsoft.Extensions.Configuration;

public abstract class BaseTestClass
Expand Down
1 change: 0 additions & 1 deletion ...ApplicationInsights.AspNetCore.Tests/Extensions/ApplicationInsightsServiceOptionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
using Microsoft.ApplicationInsights.Extensibility.PerfCounterCollector.QuickPulse;
using Microsoft.ApplicationInsights.WindowsServer;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Hosting.Internal;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.DependencyInjection.Test;
Expand Down
9 changes: 0 additions & 9 deletions ...pplicationInsights.AspNetCore.Tests/Microsoft.ApplicationInsights.AspNetCore.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,6 @@
<PackageReference Include="Moq" Version="4.16.1" />
</ItemGroup>

<ItemGroup Condition="'$(IsNetCore)' == 'True'">
<!-- TODO: Can't switch to FrameworkReference yet;
'IHostingEnvironment' is obsolete: 'This type is obsolete and will be removed in a future version.
The recommended alternative is Microsoft.AspNetCore.Hosting.IWebHostEnvironment.' -->
<!--<FrameworkReference Include="Microsoft.AspNetCore.App" />-->

<PackageReference Include="Microsoft.AspNetCore.Mvc.Core" Version="2.2.5" />
</ItemGroup>

<ItemGroup>
<None Update="App.config">
<CopyToOutputDirectory>Always</CopyToOutputDirectory>
Expand Down
1 change: 0 additions & 1 deletion ....AspNetCore.Tests/TelemetryInitializers/AspNetCoreEnvironmentTelemetryInitializerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
{
using Microsoft.ApplicationInsights.AspNetCore.TelemetryInitializers;
using Microsoft.ApplicationInsights.DataContracts;
using Microsoft.AspNetCore.Hosting.Internal;
using Xunit;

public class AspNetCoreEnvironmentTelemetryInitializerTests
Expand Down
Loading