-
Couldn't load subscription status.
- Fork 123
Use framework-specific dependencies for .NET Framework versions #4505
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.
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
base: main
Are you sure you want to change the base?
Conversation
…uGet package. Removed TransientDependenciesTests - it should be rewritten later.
…ation.Assemblies.NetFramework It partially resolve open-telemetry#4520 by making both NuGet and zip version of OpenTelemetry.AutoInstrumentation reference same versions of dependencies, but not change assembly redirections or versions included in zip. Switched DependencyListGenerator to use dotnet list package internally. Validate defined PackageVersion through MsBuild evaluation.
build/Build.Steps.cs
Outdated
| private string MapToFolderOutput(TargetFramework targetFramework) | ||
| { | ||
| return targetFramework.ToString().StartsWith("net4") ? "netfx" : "net"; | ||
| return targetFramework.ToString().StartsWith("net4") ? $"netfx" : "net"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return targetFramework.ToString().StartsWith("net4") ? $"netfx" : "net"; | |
| return targetFramework.ToString().StartsWith("net4") ? "netfx" : "net"; |
| public static readonly TargetFramework NET462 = new() { Value = "net462" }; | ||
| public static readonly TargetFramework NET47 = new() { Value = "net47" }; | ||
| public static readonly TargetFramework NET471 = new() { Value = "net471" }; | ||
| public static readonly TargetFramework NET472 = new() { Value = "net472" }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need net48 and net481 too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently we don't need net48 and net481, as all of our dependencies does not have difference between 4.7.2 and higher. I've added new NetFrameworkDistroTests.ReferencedPackagesNoUnsupportedTfm test that validates it. If any dependencies will do customizations for 4.8, 4.8.1 or potential newer .Net Framework - it will fail. There is comment in tests what should be done in that case.
As was described in the SIG meeting, I think falling back to 4.6.2 makes sense to maintain the existing the behavior. However, I'm wondering if we can improve upon it for those using the install script? Maybe the install script is run with enough permissions to configure an environment variable to modify the fallback behavior if a newer version of .net framework is detected at install time. If so, something like that could be implemented separately. |
| int frameworkVersion = 462; | ||
|
|
||
| HKEY hKey = nullptr; | ||
| LONG result = RegOpenKeyExW(HKEY_LOCAL_MACHINE, L"SOFTWARE\\Microsoft\\NET Framework Setup\\NDP\\v4\\Full\\", 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The registry check logic is in multiple locations. Is it possible to just have it checked in one place and made available in both locations?
One idea for a set of integration tests is having a test app that runs in windows containers with only a specific .net framework version available to make it easier to validate behavior for each specific .net framework version. I don't know what type of test app we need to have to ensure that the correct dependencies are in place. I also don't know if these tests will have enough value given how long it can take to download and run these containers. |
@nrcventura, I will do .Net Framework 4.7.2 detection based on presence of ICorProfilerInfo8. We will fall back to registry check only for 4.6.2, 4.7, 4.7.1. If we ever need support different packing for 4.8 or newer, we will need to reconsider it. On other hand, by that time we probably would not need anything older than 4.7.2 - as 4.6.2 - 4.7.1 probably will be out of support after Jan 12, 2027. |
@nrcventura, it may be hard to do it. To test older frameworks, docker should be based on Windows Server 2016 (as Windows Server 2019 has 4.7.2 pre-installed). As I understand (I'm not sure about it), windows docker in Github CI should match host version - and looks like there is no Windows Server 2016 runner. I also have concerns about value of this tests and whether their development is proper time investment, with limited lifespan of frameworks older than 4.7.2 (both .Net Framework 4.6.2 and Windows Server 2016 extended support will be finished on January 12, 2027) |
Use ICorProfilerInfo8 for .NET Framework 4.7.2 detection. Use PInvoke for framework resolution in loader. AssemblyResolver make non-static to allow usage of log in ctor.
|
Note When merged, close also #3817 |
| - .NET Framework only, following packages updated | ||
| - `OpenTelemetry.Instrumentation.AspNet` from `1.12.0-beta.1` to `1.13.0-beta.2`. | ||
|
|
||
| #### Dependencies on .Net Framework |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #### Dependencies on .Net Framework | |
| #### Dependencies on .NET Framework |
| #### Dependencies on .Net Framework | ||
|
|
||
| When OpenTelemetry .NET AutoInstrumentation is compiled for .NET Framework, | ||
| it uses the net462 Target Framework Moniker (TFM). As a result, the ZIP archive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| it uses the net462 Target Framework Moniker (TFM). As a result, the ZIP archive | |
| it uses the `net462` Target Framework Moniker (TFM). As a result, the ZIP archive |
| string[] arguments = new[] | ||
| { | ||
| "list", $"\"{projectPath}\"", "package", "--include-transitive", "--format", "json", "--output-version", | ||
| "1" | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| string[] arguments = new[] | |
| { | |
| "list", $"\"{projectPath}\"", "package", "--include-transitive", "--format", "json", "--output-version", | |
| "1" | |
| }; | |
| string[] arguments = | |
| [ | |
| "list", $"\"{projectPath}\"", "package", "--include-transitive", "--format", "json", "--output-version", "1" | |
| ]; |
|
|
||
| using System.Text.Json.Serialization; | ||
|
|
||
| public record PackageListModel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.NET provides some wrappers to play with nugets, and other stuff.
Eg https://www.nuget.org/packages/NuGet.ProjectModel
Is there any chance that there is similar package for this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please share the new/old package sizes on Windows?
| \netfx\net471\System.Security.SecureString.dll, | ||
| \netfx\net471\System.Threading.Overlapped.dll, | ||
| \netfx\net471\System.Xml.XPath.XDocument.dll, | ||
| \netfx\net472\_._, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This look strange. What that means?
| // Map release numbers to framework versions | ||
| // Based on Microsoft documentation: | ||
| // https://docs.microsoft.com/en-us/dotnet/framework/migration-guide/how-to-determine-which-versions-are-installed | ||
| if (releaseValue >= 461308) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put here all framework versions, up to 4.8.1.
in if statement, we can put something like
// intentionally pinining to .NET Framework 4.7.2, there is no differences in the expected library set
frameworkVersion = 472;
or similiar.
Now, we understand that it was intentional, in the future we can avoid some archeological stuff.
|
|
||
| $release = Get-ItemPropertyValue -LiteralPath 'HKLM:SOFTWARE\Microsoft\NET Framework Setup\NDP\v4\Full' -Name Release | ||
| switch ($release) { | ||
| { $_ -ge 461808 } { $fxversion = 'net472'; break } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put all versions also here, similar comment as in cor_profiler.cpp
| Uninstall-OpenTelemetryCore | ||
| ``` | ||
|
|
||
| #### Update .Net Framework version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #### Update .Net Framework version | |
| #### Update .NET Framework version |
Do you know what can be the most common case when this can happen? Do you see any other possibility to check what .NET Framework is executed?
Historically, it prevented me at least once to merge wrong PR. If you see possibility to still have it (this is pretty new feature in .NET) it will be great. I think, it can be done in separate PR, if you can commit to finish it;
OpenTelemetry.Api.ProviderBuilderExtensions always has the same version as OpenTelemetry.Api/OpenTelemetry I do not have strong opinion here.
I would say, that we should switch whole repository to renovate first, then make any additional inclusions. |
Why
Currently OTEL registers in GAC assemblies, that was never intendend to be put in GAC on latest framework - and not registering netstandard.dll on 4.6.2. It results in issues discussed at #4186 (comment)
What
With this change, we pack different set of assemblies for supported .NET Frameworks:
Only assemblies for current framework will be registered in GAC at install time or loaded from installation dir.
To minimize size set of assemblies deduplicated - with assemblies that used in all .NET Frameworks still stay at
netfxfolders. Assemblies used only in some versions stays in oldest TFM folder. For other TFM using same assembly .link file will be created that points to TFM with assembly.To simplify buget packages tracking
CentralPackageTransitivePinningEnabledfor newly created OpenTelemetry.AutoInstrumentation.Assemblies.NetFramework.csproj was enabled - with it transient auto-generated packages were removed fromOpenTelemetry.AutoInstrumentation.csproj..NET Framework version resolved through registry check in installation, profiler and managed code. It associated with some risk (proper rights). If version will not be resolved, it is fallback to 4.6.2, which should have same behaviour as before this change. It may be better nowdays to fallback to 4.7.2 - as most people probably have it.
Deployment for .Net Frameworks higher than 4.7.2 are same as for 4.7.2, so we do not build them - but test added to validate it.
Tests
Checklist
CHANGELOG.mdis updated.Open questions
Should we have it? Currently they are added as other transitive dependencies in OpenTelemetry.AutoInstrumentation.Assemblies.NetFramework/Directory.Packages.props. We can prevent it by restoring !dep.Name.StartsWith("OpenTelemetry") && dep.Name != "OpenTracing" in Generator, but pinning them through higher level Directory.Packages.props may be better idea. Packages to discuss: