opentelemetry-dotnet icon indicating copy to clipboard operation
opentelemetry-dotnet copied to clipboard

[bug] targets other than net9.0 shouldn't be forced to use 9.0 dependencies

Open paulomorgado opened this issue 1 year ago • 56 comments

Package

OpenTelemetry

Package Version

Versions 1.10.0 and those that take dependencies on it.

Runtime Version

net8.0, netstandard2.1, netstandard2.0, net462

Description

.NET 8.0 and the libraries tied to it will be supported until November 10, 2026 while .NET 9.0 and the libraries tied to it May 12, 2026.

Forcing users that want or need to be on the long term support channel to move to .NET 9.0 will expose them to possible breaking changings now and, on May 12, 2026 and November 10, 2026.

Given that OpenTelemetry is a cross-cutting concern and touches every application and service, forcing .NET 9.0 libraries on users that want or need to be on the long term support channel will prevent them from using the latest OpenTelemetry libraries.

Steps to Reproduce

Create an Aspire starter application targeting .NET 8.0 and upgrade OpenTelemetry components to the latest versions.

Expected Result

.NET 9.0 libraries is not forced upon the services.

Actual Result

.NET 9.0 libraries is forced upon the services.

Additional Context

I'm willing to send a PR to fix this.

paulomorgado avatar Nov 13 '24 09:11 paulomorgado

There's a lot of context here that's important to understand. I'm sure that others will provide a bit more deeper analysis.

At a top level, there are some new APIs in .NET that we require in order to achieve compatibility with the spec. There are around the new Metrics functionality in .NET 9, which is linked to the Diagnostics libraries. Without those, we wouldn't be able to support the required functionality, which is ultimately the reason we've had to drop support for .NET 6.

The System.Diagnostics 9.x libraries work in .NET 8 solution, so they won't need to upgrade to .NET 9. Therefore people will be able to be on the LTS of .NET, and benefit from the new libraries.

martinjt avatar Nov 13 '24 10:11 martinjt

OpenTelemetry always uses latest System.Diagnostics library because it utilizes its newly added API, it's seems OK - the diagnostics API is very stable and it's required to implement OTEL features. But for Microsoft.Extensions.* it's probably better to use <FrameworkReference Include="Microsoft.AspNetCore.App /> as suggested by MS docs (https://learn.microsoft.com//aspnet/core/fundamentals/target-aspnetcore?view=aspnetcore-9.0&tabs=visual-studio#use-the-aspnet-core-shared-framework) because it helps to avoid some breaking changes like 8.0.x regression with keyed services + IServiceCollection introspection in app/3rd-party libraries (fixed in latest 8.0.x releases).

vladislav-prishchepa avatar Nov 13 '24 10:11 vladislav-prishchepa

I know Microsoft.Extensions.* and System.Diagnostics.* version 9 can be used on .NET 8.0, but there's a supportability issue you are imposing on users that you can't do.

.NET 8.0 didn't change to use .NET 9.0 libraries targeting .NET 8.0 and there are reasons for that.

I suspect you'll change this very soon.

paulomorgado avatar Nov 13 '24 10:11 paulomorgado

@davidfowl, what are your plans regarding this and Aspire ServiceDefaults? Will .NET 9.0 libraries be forced upon users?

paulomorgado avatar Nov 13 '24 10:11 paulomorgado

There will be things that will have to change since you can't use the new Meter functionality without the Diagnostics information.

What I would imagine is that 99.9% of users will not notice anything. They'll pull in the latest OpenTelemetry and that will pull in the latest System.Diagnostics and everything will work. It's rare, in my experience, for someone to reference System.Diagnostics directly, and even if they do, it will resolve to the latest.

I can't really see a situation where this would be an issue, even in security conscious organisations (which I work with regularly). The only ones affected by this change are the .NET 6 users who can't upgrade to .NET 8.

martinjt avatar Nov 13 '24 10:11 martinjt

OK! I see you have no idea of the impact of this. When I have time I'll post here the dependency graph.

paulomorgado avatar Nov 13 '24 10:11 paulomorgado

I'd be happy to hear about what issues you see with this, we've spent a lot of time discussing this and working how we ensure we don't end up in the same situation we've had previously.

I worked through a tonne of scenarios prior to very long discussion the SIG had on how to do it, and .NET 6 is the only scenario that actually becomes an issue.

To be clear, Aspire.ServiceDefaults wouldn't need to take a dependency directly System.Diagnostics 9.0.0.0, I don't think it takes a direct dependency on 8 right now (I'm currently updating to check that). It will take an indirect dependency on that by taking OpenTelemetry 1.10 (released yesterday), it will also take that dependency when it takes on some of the instrumentations that need access to the new Meter APIs.

None of that, from what we investigated, would stop someone staying on 8.0.0.0. Even In-Process functions won't be an issue since they're already stuck on the previous versions of Otel.

So if you have a situation you can articulate where there is an issue, we can absolutely look into it, but we've spent a tonne of time working this out and didn't find anything that would be considered insurmountable at an organisation level, unless they're in a mode where they can't take BCL libraries beyond 8.0.0.0 which is a very different issue.

You could also join the SIG meeting next week to talk to us about it.

martinjt avatar Nov 13 '24 10:11 martinjt

@martinjt, you keep mentioning System.Diagnostics 9.0.0.0, but if you look at Directory.Packages.props, you'll see these:

<Project>

  <PropertyGroup>
    <!-- ... -->
    <LatestRuntimeOutOfBandVer>9.0.0</LatestRuntimeOutOfBandVer>
    <!-- ... -->
  </PropertyGroup>

  <!--
      This section covers packages that are directly referenced by the NuGet packages published from this repository.
      Any security vulnerability in these packages or their downstream dependencies will be considered as a security
      vulnerability in the NuGet packages that are published from this repository.
  -->
  <ItemGroup>
    <!--
        Typically, for the Microsoft.Extensions.* packages relating to DI Abstractions, Hosting Abstractions, and Logging,
        the latest stable version should be used because:
        1) Each major version bump will have some new API capabilities (e.g.For Logging, .NET 6 introduced compile-time logging
          source generation, .NET 8 introduced automatic event id generation).
        2) Each minor version bump is normally security hotfixes or critical bug fixes.
        3) Since version 3.1.0, the .NET runtime team is holding a high bar for backward compatibility on
          these packages even during major version bumps, so compatibility is not a concern here.
    -->
    <PackageVersion Include="Microsoft.Extensions.Configuration" Version="$(LatestRuntimeOutOfBandVer)" />
    <PackageVersion Include="Microsoft.Extensions.DependencyInjection.Abstractions" Version="$(LatestRuntimeOutOfBandVer)" />
    <PackageVersion Include="Microsoft.Extensions.Diagnostics.Abstractions" Version="$(LatestRuntimeOutOfBandVer)" />
    <PackageVersion Include="Microsoft.Extensions.Hosting.Abstractions" Version="$(LatestRuntimeOutOfBandVer)" />
    <PackageVersion Include="Microsoft.Extensions.Logging" Version="$(LatestRuntimeOutOfBandVer)" />
    <PackageVersion Include="Microsoft.Extensions.Logging.Configuration" Version="$(LatestRuntimeOutOfBandVer)" />
    <PackageVersion Include="Microsoft.Extensions.Options" Version="$(LatestRuntimeOutOfBandVer)" />
    
    <!-- ... -->

    <!--
        Typically, the latest stable version of System.Diagnostics.DiagnosticSource should be used here because:
        1) Each major version bump will likely have some new OpenTelemetry capabilities (e.g. .NET 6 introduced Meter
          API, .NET 7 added UpDownCounter, .NET 8 added Meter/Instrument level attributes support, .NET 9 added
          Advice/Hint API, etc.).
        2) Each minor version bump is normally security hotfixes or critical bug fixes.
        3) The .NET runtime team provides extra backward compatibility guarantee to System.Diagnostics.DiagnosticSource
          even during major version bumps, so compatibility is not a concern here.
    -->
    <PackageVersion Include="System.Diagnostics.DiagnosticSource" Version="$(LatestRuntimeOutOfBandVer)" />

    <!-- ... -->

  <!--
      This section covers packages that are **not** directly referenced by the NuGet packages published from this repository.
      These packages are referenced as "PrivateAssets" or used in tests/examples.
  -->
  <ItemGroup>
    <!-- ... -->
    <PackageVersion Include="Microsoft.Extensions.DependencyInjection" Version="$(LatestRuntimeOutOfBandVer)" />
    <PackageVersion Include="Microsoft.Extensions.Hosting" Version="$(LatestRuntimeOutOfBandVer)" />
    <PackageVersion Include="Microsoft.Extensions.Http" Version="$(LatestRuntimeOutOfBandVer)" />
    <PackageVersion Include="Microsoft.Extensions.Logging.Abstractions" Version="$(LatestRuntimeOutOfBandVer)" />
    <PackageVersion Include="Microsoft.Extensions.Telemetry.Abstractions" Version="[9.0.0,)" />
    <!-- ... -->
  </ItemGroup>

  <ItemGroup Condition="'$(TargetFramework)' == 'net8.0'">
    <PackageVersion Include="Microsoft.AspNetCore.TestHost" Version="8.0.8" />
  </ItemGroup>

  <ItemGroup Condition="'$(TargetFramework)' == 'net9.0'">
    <PackageVersion Include="Microsoft.AspNetCore.TestHost" Version="9.0.0" />
  </ItemGroup>

</Project>```

You are explicitly imposing upgrades from LTS to STS for 10+ NuGet packages, and thus for all their dependencies. And those include configuration and dependency injection but also hosting.

paulomorgado avatar Nov 13 '24 11:11 paulomorgado

I mention System.Diagnostics as that's the reason for the upgrade, and ultimately the one that will cause issues.

You'll see from the comments about the advice on these packages. Like I said though, I'm curious about the scenario that would stop someone from upgrading these packages. I've not come across organisations that have issues with package updates like this, only runtime updates.

I do find it curious that there are organisations that will take a dependency on an OpenSource project like OpenTelemetry, which is run by volunteers, but won't upgrade dependencies like Microsoft.Extensions.* or System.* when Microsoft are maintaining the backwards compatibility.

Like I said, if you have those scenarios, I'm more than happy to discuss them. However, please don't think this was a decision that we took likely, there was weeks of contentious debate and investigation of the impacts (checkout the notes from the SIGs from September).

martinjt avatar Nov 13 '24 11:11 martinjt

The scenario is a company with the mandate to stay on LTS dependencies.

paulomorgado avatar Nov 13 '24 11:11 paulomorgado

In this scenario, how are they getting around the fact that OpenTelemetry itself doesn't have an LTS agreement? That's the part that I'm struggling to justify, that they'll take third party dependencies that don't have 2 year official support programs, but won't allow the BCL to be upgraded.

I've absolutely come across vetted libraries, and not upgrading the runtime, but I've never come across BCL upgrades being an actual. That's across Healthcare, Banking and Insurance, which are the most restrictive I've come across.

The most secure I've come across is "nothing outside the BCL", but that would rule out using OTel in general.

I'd love to hear a real-world example with the wording of the internal policy. This was, essentially, the missing part from the discussion where I ultimately conceded that 1.10 would take the upgrade.

martinjt avatar Nov 13 '24 12:11 martinjt

There was a discussion about this sort of thing over in the dotnet/extensions repository recently: https://github.com/dotnet/extensions/issues/5462

The context there may not apply here, but it might be a useful data point for people to take a look at here on the "using 9.0 libraries in a 8.0 application" aspect. The TL;DR in that case is that it impacts the deployed application size as additional dependencies that would otherwise be in the shared framework have to instead be shipped with the app code.

I'm not planting a flag in either side of the discussion here, I just thought you might find the above useful.

martincostello avatar Nov 13 '24 14:11 martincostello

@martincostello, I see that as a different use case. If the user decides to pick 9.0.0, the user gets 9.0.0.

When the user is picking OpenTelemetry is 1.10.0, it's not obvious that the only choice is to have .NET 9.0 libraries.

paulomorgado avatar Nov 13 '24 16:11 paulomorgado

For the user, yes. The comment was more for the maintainers to look at and take into consideration.

martincostello avatar Nov 13 '24 16:11 martincostello

@paulomorgado do they currently care about the version of Google Protobuf that is being used? or the grpc package?

I'm asking this specifically because there are more dependencies than just the System.* and Microsoft.Extensions.* so I'm trying to understand why those would be excluded from the business's security policy on LTS vs STS?

martinjt avatar Nov 13 '24 16:11 martinjt

@martinjt, if anything has LTS and STS policies, users/companies/clients shouldn't be forced out of the chosen path.

paulomorgado avatar Nov 13 '24 16:11 paulomorgado

I'm not sure I understand the distinction in that comment? Since there is no STS/LTS path for OpenTelemetry, they're already being forced out of that path to use OpenTelemetry at all.

In that scenario, what would stop them from staying on 1.9 with their 8.0.0.0 dependencies, what's the forcing factor pushing them to 1.10?

(and to be clear, I'm not trying to be antagonistic here, it's a useful conversation as these are the examples we had when we discussed the path in the SIG and we couldn't find a realistic example that we should consider).

martinjt avatar Nov 13 '24 16:11 martinjt

If a security issue is found on OpenTelemetry 1.9.0, will it be fixed, or will it require to move to 1.10.x?

paulomorgado avatar Nov 13 '24 16:11 paulomorgado

We did discuss this and the consensus was a solid "maybe" but definitely not a "no". This was more around the need for 6.0 than to do with organisations using 8.0, but I would say it would still apply.

There's no reason we couldn't setup an interim 1.9.1 if that did happen and it was serious enough.

martinjt avatar Nov 13 '24 16:11 martinjt

I'll clarify this with my team, but my take is this:

  • System.Diagnsotics.DiagnosticSource is specifically designed to replace the in-box component. If you're on .NET 8 and you use S.D.DS 9.0 your're still supported.
  • We generally want OpenTelemetry to use the lastest version because we added features to the package in support of that.

terrajobst avatar Nov 13 '24 19:11 terrajobst

@terrajobst

I'll clarify this with my team, but my take is this:

  • System.Diagnsotics.DiagnosticSource is specifically designed to replace the in-box component. If you're on .NET 8 and you use S.D.DS 9.0 your're still supported.
  • We generally want OpenTelemetry to use the lastest version because we added features to the package in support of that.

Does that extend to Microsoft.Extensions.*?

Neither will be supported after 9.0 goes out of support and 8.0 is still supported, right.?

paulomorgado avatar Nov 13 '24 20:11 paulomorgado

Neither will be supported after 9.0 goes out of support and 8.0 is still supported, right.?

Right. By that time (9.0 goes out), a new version of OpenTelemetry would be available which brings 10.0 packages, and it will still support net8.0 applications.

reyang avatar Nov 13 '24 22:11 reyang

Sorry, we have BIG Problems with that. It simply does not work for us with .NET 8 (we would move to .NET 9, but we have not all external dependencies ready yet, Oracle will hopefully be ready next month)

At first glance, we just got NuGet Restore Downgrade errors. And because these are just Extensions (and have .NET 8 build target, so they should work). I decided to give it ago.

I updated our internal NuGet Packages to 9.0 Extensions, that worked, but not all have direct dependencies, and are not newly created.

Now we had to update our Project, i tried two not that small Projects also update to 9.0 Microsoft.Extensions (and System.Text), together with the packages. The Compile. But they won't start, nor run our low-level Integration Tests are working. Because of some Dependency Injection Problems.

  • Now we have to investigate the Issue. How we may, could move forward
  • We have to deal with our Automated Dependency Management because this minor update is breaking all of our things.
  • I think many other people/company have to deal with that.

I would assume some Binary Compatibility Issue on the way up to the Application because not every other external dependency has to be recompiled to use the 9.0 Extensions, and just forced to link against 9.0. And some level higher up it breaks. An Example for that: https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/issues/4789

EDIT:

Tried two Scenarios with these two Projects:

  1. Just updated Microsoft.Extension/System.Text to 9.0.0 (without our Nuget Packages) with Central Package Management. (not even the new OT Package was included)
  2. Updated Extensions Packages and our own Nuget Packages (which includes the OT Packages)

Both have the same problem, so the root cause for the IoC Problems is a level lower than the OT package.

DerAlbertCom avatar Nov 14 '24 07:11 DerAlbertCom

Forcing users that want or need to be on the long term support channel to move to .NET 9.0 will expose them to possible breaking changings now and, on May 12, 2026 and November 10, 2026.

@reyang, from my opening post:

Forcing users that want or need to be on the long term support channel to move to .NET 9.0 will expose them to possible breaking changings now and, on May 12, 2026 and November 10, 2026.

paulomorgado avatar Nov 14 '24 08:11 paulomorgado

Does that extend to Microsoft.Extensions.*?

There are Microsoft.Extensions.* and there are Microsoft.Extensions.*. There are also Microsoft.Extensions.*.... The first are coming from dotnet/runtime, and those covered by one support policy. Then there are others coming from dotnet/aspnetcore, and those are covered by their own policy (which may not differ wildly from the dotnet/runtime's). Then the last lost is coming from dotnet/extensions, which has its own support policy.

RussKie avatar Nov 14 '24 09:11 RussKie

Does that extend to Microsoft.Extensions.*?

There are Microsoft.Extensions.* and there are Microsoft.Extensions.*. There are also Microsoft.Extensions.*.... The first are coming from dotnet/runtime, and those covered by one support policy. Then there are others coming from dotnet/aspnetcore, and those are covered by their own policy (which may not differ wildly from the dotnet/runtime's). Then the last lost is coming from dotnet/extensions, which has its own support policy.

@RussKie, would mind giving concrete examples that have different time frames for the same channel?

paulomorgado avatar Nov 14 '24 11:11 paulomorgado

I tried to read through the various support policies listed above and ended up getting linked from one to a different one and then mixing up which applied to what.

@terrajobst It would be great to include in the platform support policy how the M.E.* packages included in dotnet/runtime are supported as this can broadly be a misunderstood issue. I imagine that space/attention is limited on what gets put on that main policy page, but this seems like it's important enough to be included.

If running on an LTS runtime with the same or newer M.E.* extension package is supported through the LTS timeline, and that can be demonstrated through documentation to leadership or whoever is enforcing the policy, then that may be all that's needed to address the concerns brought up in this issue.

rkargMsft avatar Nov 16 '24 01:11 rkargMsft

EDIT:

Tried two Scenarios with these two Projects:

  1. Just updated Microsoft.Extension/System.Text to 9.0.0 (without our Nuget Packages) with Central Package Management. (not even the new OT Package was included)

@DerAlbertCom were you able to scope this down and have a self-contained minimal repro? If yes, I think that's something the .NET team can help.

reyang avatar Nov 16 '24 01:11 reyang

@reyang no, it's a complex software setup (we may even be doing something wrong, but the exception is not helpful) and I doubt that my company will support this (putting hours or days of work of hunting down this bug).

It is not an OT-specific problem, the OT problem here is forcing users to move to the 9.0.x Version of the Extensions which breaks existing Applications (I don't complain about the Support Lifecyle).

I can provide further Information you request, be willing to run something to gather more Information for you. Like seeing which dependencies are finally being used in the application.

We hope this will be fixed by itself when we move completely to .NET 9. But this is clearly lot a drop-in replacement. In a mixed use (8.0.x and 9.0.x Extensions) environments.

More here: https://github.com/dotnet/extensions/issues/5644#issuecomment-2480524930

DerAlbertCom avatar Nov 16 '24 11:11 DerAlbertCom

It seemed that JUST the Dependency Validation was the problem (first impression after giving it some investigation), then I dug a little.

Then I located the issue in our code, this breaking change in 9.0.0.

https://learn.microsoft.com/en-us/dotnet/core/compatibility/aspnet-core/9.0/hostbuilder-validation (we can fix that)

But still, this minor Version OpenTelemetry Update breaks builds.

DerAlbertCom avatar Nov 16 '24 12:11 DerAlbertCom