Consider decoupling EnvariableVariable implementation from the SDK

[reyang]

Bug Report

OpenTelemetry.dll 1.4.0-rc2 has dependency on Microsoft.Extensions.Configuration.EnvironmentVariables. The suggestion is to move the environment variable implementation and related dependencies to a separate package. The main reason is that environment variable is just one way of doing configurations, later there might be file-based solution or other solutions - keeping them separate will allow the core package to have low footprint, with reduced attack surface and smaller SBOM (Software Bill of Material) - these are important in building trustworthy software.

Consider this as a release blocker for 1.4.0.

https://www.nuget.org/packages/OpenTelemetry/1.4.0-rc.2#dependencies-body-tab

[cijothomas]

#3720 (comment)

[CodeBlanch]

Here is more information about this change...

History:

• 1.3.1 SDK uses System.Environment directly. Example: BatchExportActivityProcessorOptions which calls into EnvironmentVariableHelper.

• 1.4.0 SDK uses Microsoft.Extensions.Configuration.IConfiguration. Example: BatchExportActivityProcessorOptions.

The reason for this is precisely what the description is describing, there are lots of ways to do configuration beyond environment variables. Supporting IConfiguration decouples the SDK from any particular source. One example of this being requested: #2980.

The reason for the Microsoft.Extensions.Configuration.EnvironmentVariables dependency is backwards compatibility:

• .NET Framework users (more precisely users not using hosting) before had support for environment variables. Switching to IConfiguration would break that. In order to preserve the behavior the SDK will create configuration from environment variables if it isn't found.

• ASP.NET Core users (more precisely users using hosting) automatically have support for IConfiguration which by default includes environment variables, command-line, & json configuration files. In this case the SDK just respects whatever IConfiguration it finds.

[reyang]

Trying to see if we can steer the communication/conversation to another direction:

1. When we start to see more and more environment variables being added, and things are getting more and more complex, what do we do? One option is to keep adding things until we fail, another option is to hit the brake and rethink about the direction (e.g. [TC Request]: Environment-based Configuration Guidance opentelemetry-specification#2891 (comment) is essentially saying "we realized that keep adding environment variables will lead us to a bad situation and we want to slow it down").
2. Do we think IConfiguration is the only way of extending configurations and .NET is betting on it, or it is one of the many possible fashions that would show up at some point and fade soon? If we follow the 1.4.0 approach 5 years ago, we would probably be stuck with System.Configuration.ConfigurationManager now - do we want to run into such situation in 5 years?

[alanwest]

Also briefly discussed here #3639 (comment)

keeping them separate will allow the core package to have low footprint, with reduced attack surface and smaller SBOM (Software Bill of Material) - these are important in building trustworthy software.

In general I completely agree. We have already violated this pretty hard though. OpenTelemetry .NET 1.3.2 depends on Microsoft.Extensions.Logging.Configuration.

I completely agree that we need to define the boundaries of what's permitted and what is not otherwise we're prone to falling further down this slippery slope.

Regarding specifically Microsoft.Extensions.Configuration.EnvironmentVariables, though, my thought was that the since we've already doubled down pretty hard on IConfiguration, it's a pretty small concession to bring in EnvironmentVariables.

another option is to hit the brake and rethink about the direction

I'm completely open to this discussion. I think it's a sticky one though. The big thing that I think we've been lacking is a concerted design effort regarding package/dependency organization. At this point it almost feels like we have to consider a 2.0 release breaking a lot of what we've already shipped as stable.

[reyang]

In general I completely agree. We have already violated this pretty hard though. OpenTelemetry .NET 1.3.2 depends on Microsoft.Extensions.Logging.Configuration.

+1, the sad reason is that .NET team confirmed that ILogger is the Logging API that the runtime will continue to push forward, and in order for ILogger to work, Microsoft.Extensions.Logging.Configuration is needed. However, I feel we should not use that as an excuse (e.g. Imagine if we introduced a memory leak, it should not make us feel any better about introducing another memory leak bug, right? Sharing the same spirit, I don't buy the argument of "we've already leaked once, it'll be less a problem if we leak in another place.) and we should avoid keep lowering the bar (the slippery slope as @alanwest mentioned).

The big thing that I think we've been lacking is a concerted design effort regarding package/dependency organization.

+1, and this reminded me of something similar #4086

At this point it almost feels like we have to consider a 2.0 release breaking a lot of what we've already shipped as stable.

I think we still have good opportunities to stop the slippery thing and fix the problems in 1.4.0.

[noahfalk]

If you want to excise the Configuration.EnvironmentVariables dependency in particular you could always populate your options class directly from calls to Environment.GetEnvironmentVariable() in the case no IConfiguration has been provided. Given that it appears to have no impact on usage you can just decide if the effort to maintain the extra code is worth it (from the discussion it sounds like you will conclude it is worth it)

In the broader sense of avoiding dependencies at large, that feels like more of a trade off on usability vs. dependencies. If you want users to be able to configure OTel via DI patterns or use IConfiguration, OPtions, etc then the dependencies on the libraries that define those things are mandatory. If you want to have one cohort of users that get barebones functionality and another group that gets the bells and whistles you could always define OpenTelemetry.Core.dll that has virtually no dependencies, and then OpenTelemetry.dll depends on Core + a bunch of other stuff to offer all the extra stuff that Core is missing. In the particular example of that environment variable configuration you could put BatchExportOptions<T> into the Core library which solely defines the properties, and BatchExportOptions stays in OpenTelemetry.dll.

If you do wind up having a 'core' offering and a 'bells and whistles' offering at some point I think it would be worth exploring not only how you make core smaller, but also how you make the bells and whistles option easier to use. In particular it takes a good 10-15 lines of startup code and 5+ unique NuGet package references to enable all the different instrumentation a typical ASP.NET Core app might want. I doubt most app authors want to think much about all the options OTel gives them and would gladly get started with a recommended default. Most will probably appreciate it the closer getting started with OTel can be to this:

Step 1: Add reference to a single NuGet package
Step 2: Add startup code

services.AddOpenTelemetry(config =>
{
   // default instrumentation is already enabled, use config APIs if you want to change it
   config.SendTelemetryTo("backend_service_address");
});

I know that level of simplicity is probably optimistic, but a meta-package with instrumentation defaults might get close.

[CodeBlanch]

Do we think IConfiguration is the only way of extending configurations and .NET is betting on it, or it is one of the many possible fashions that would show up at some point and fade soon? If we follow the 1.4.0 approach 5 years ago, we would probably be stuck with System.Configuration.ConfigurationManager now - do we want to run into such situation in 5 years?

My opinion is IServiceCollection and IConfiguration are very fine, very popular, very embraced, and perfectly meet the needs we have right now. I haven't seen an issue where anyone has asked us to support some future thing but I have seen issues asking for good support of these two things. Will new things come and go? Absolutely. How many tracing and metric APIs are in runtime? (Sorry @noahfalk 😄) If we try to build for something that might happen in the future, we will certainly fail. If we make something that is great right now it has a better chance of succeeding and surviving long enough to run into the problem of needing to adapt to new things 🤣 Consider .NET Core. Lots and lots of packages out there had to adapt. Some maybe refactored their bits to share. Some projects just went net-new. When ".NET Max" comes out--if we have done a good job of being relevant up to that point--we'll have the opportunity to figure it out how to support the new fashions of the day!

If you want to excise the Configuration.EnvironmentVariables dependency in particular you could always populate your options class directly from calls to Environment.GetEnvironmentVariable() in the case no IConfiguration has been provided.

I'm not opposed to this. It was something we discussed. There really isn't much in that package at all.

you could always define OpenTelemetry.Core.dll that has virtually no dependencies

I don't think anything we have done precludes that. But I'll take a stab at making it so and see if there are any blocks I run into.

[alanwest]

in order for ILogger to work, Microsoft.Extensions.Logging.Configuration is needed

I've become less convinced that OpenTelemtry.dll requires ILogger deps in order for ILogger to work. It now seems pretty compelling to have shipped ILogger support similar to @CodeBlanch's prototypes for Serilog and EventSource. That is, a separate ILogger OTel shim package would be shipped. That package would have the ILogger deps.

config.SendTelemetryTo("backend_service_address");

❤️. Many users just want telemetry without the burden of thinking about traces, metrics, logs.

[reyang]

If we try to build for something that might happen in the future, we will certainly fail. If we make something that is great right now it has a better chance of succeeding and surviving long enough to run into the problem of needing to adapt to new things 🤣

Looks like this the fundamental difference in our belief systems. I feel there is a difference between a library and a framework (OpenTelemetry SDK to me is library rather than framework, it can provide framework integrations as extensions to the SDK though). ASP.NET Core and Microsoft.Extensions.Configuration both sound like frameworks to me. Taking MFC for example, it had a goal of simplifying the developer experience for Win32 APIs, it has been trying to add more features to "make things great at the very moment", and it became too big and it died. Similarly, almost all frameworks come and die every 5 or 10 years (e.g. think about Ruby on Rails, MVC, Web Api, WinForm, WCF, AngularJS, React).

[reyang]

in order for ILogger to work, Microsoft.Extensions.Logging.Configuration is needed

I've become less convinced that OpenTelemtry.dll requires ILogger deps in order for ILogger to work. It now seems pretty compelling to have shipped ILogger support similar to @CodeBlanch's prototypes for Serilog and EventSource. That is, a separate ILogger OTel shim package would be shipped. That package would have the ILogger deps.

+1 I don't know how this can be achieved at this moment, but we should "try to fix" it (of course in a non-invasive way) and also stop the trend of "there was already such a mistake, so let's just add three more".

[CodeBlanch]

Has anyone raised an issue other than @reyang about the dependencies? We forced Microsoft.Extensions.Logging a long long time ago, did anyone complain? I linked 9 issues above people asking for deep integration and there are more. We could go and blow things up and re-layout everything so there is a happy dependency graph but who are we satisfying by doing that? Is there a true ask/need for this "Core" package?

[pellared]

Has anyone raised an issue other than @reyang about the dependencies?

I hope we did but probably not well enough 😸 So maybe I will try to explain the biggest issue from my POV.

From https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation standpoint the dependencies are a big (maybe even the biggest) issue as we might have conflict on the application's and SDK's version of the same dependency. We are continuously dealing with this kind of issues...

Reference: https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/blob/main/docs/troubleshooting.md#assembly-version-conflicts

To mitigate this kind of problems Datadog, New Relic auto-instrumentations are "vendoing" the dependencies' code like here: https://github.com/DataDog/dd-trace-dotnet/tree/master/tracer/src/Datadog.Trace/Vendors

CC @open-telemetry/dotnet-instrumentation-maintainers

[martinjt]

I don't see this being something that we should be solving in the 1.x stream, let alone blocking a new release because of it.

We're in a REALLY bad state with dependencies and overall confidence of OpenTelemetry.NET right now. Users are asking for easier ways to do telemetry and the continuous halting of the RCs, and adding more scope, etc. is not helping. I'm fielding comments constantly about it not working, and having to overcome objections about it not being stable and supported. This is pushing people back to Vendored frameworks like Application Insights. I get that's good for Microsoft but it's bad for overall adoption of OTel which is the goal for this project. We're not bound by the same release change rate of the BCL, we can release a v2, we can remove them in 1.5 if we need to. Blocking the release shouldn't be considered here.

I would be supportive of a collaborative redesign of the library structure as part of a 2.x release stream to think about all the things discussed here.

On some specific points.
Microsoft.Extensions.Configuration is used in pretty much every application that I've seen and I spend a lot of time with different types of developers from Web to Console, Event etc. Adding it as a dependency here makes it more idiomatic to the majority of users. Removing it is hobbling the majority of users in favour of purity.

On general dependencies increasing bloat, especially in auto-instrumentation, the big use case there is people instrumenting where there are containers, they're already likely bringing in SqlClient and ASP.NET Core, both are big. I highly doubt that adding that new dependency is going to affect things more than those are already doing it. I don't know too much about the auto-instrumentation library though.

What this feels like to me is that we're trying to do too much, and at the 11th hour after months of work on it to raise this now seems like not a great look.

In short, move this discussion of "Core" and "SDK" modularity and the existential debate of philosophically what is a framework and what's an SDK into a 2.x release stream and move forward with 1.4 that's stable right now so we can give the community confidence that we can release stable things.

[cijothomas]

We're in a REALLY bad state with dependencies and overall confidence of OpenTelemetry.NET right now

? I do not understand this part. Would you elaborate with specifics, in a separate issue?

[cijothomas]

I get that's good for Microsoft but it's bad for overall adoption of OTel which is the goal for this project

Any particular reason why you make this statement? Do you see Microsoft is doing anything to negatively impact OTel adoption?

[martinjt]

We're in a REALLY bad state with dependencies and overall confidence of OpenTelemetry.NET right now

? I do not understand this part. Would you elaborate with specifics, in a separate issue?

We've discussed this a lot in the slack channels. From the dependency issues that people are having (I think we now have a way forward on this with the tentative agreement around 0.x releases) through to the issues with application platforms like Azure functions struggling with adopting it and even Microsoft not releasing the Azure.Monitor exporter as final. It's a pattern that I think people are saying.

I can only say what I hear at the coal face dealing with people's objections during and after community talks about Otel and .NET. People are excited, but won't adopt it because they feel it's not production ready, not stable, etc.

It's holding back releases based on idealogy, and doing it at the 11th hour that erodes confidence.

Take the 1.4 release. The simplification and unification it brings to setup, we're now looking at removing that because of idealogy. As a .NET Framework thing, I can understand, but you know from our chats that we have to balance being idiomatic to the language/runtime, and the overall OTel standards. OpenTelemetry standards and being easy to adopt are the 2 most important factors, not matching how the .NET team treat SDKs and Frameworks.

The perception people have of the MS team, and how they treat free features is part of the problem, as there are so many Microsoft people involved. This particular blocker will be perceived the same way. This isn't a Microsoft project, and shouldn't be treated that way.

[cijothomas]

the dependency issues that people are having (I think we now have a way forward on this with the tentative agreement around 0.x releases)_

The fact that the instrumentation libraries are not stable (due to sem. conventions not being stable) is independent of this issue.

issues with application platforms like Azure functions struggling with adopting it

Azure Functions have some hard dependency on DS versions, and it is independent of OpenTelemetry, and existed even before OpenTelemetry project started. Its a known issue in Azure Functions, and it is unrelated to this issue being discussed here.

even Microsoft not releasing the Azure.Monitor exporter as final._

How is that related here? Azure Monitor stable is not blocked by anything from this repo. (I am one of the owners of AzureMonitor package).

This isn't a Microsoft project, and shouldn't be treated that way.

It is not.

[martinjt]

So that's 4 separate issues that are related to OpenTelemetry and its stability, but that has nothing to do with the overall perception that our customers (.NET Developers) are talking about.

That's the issue we have, it's being treated like a purely technical, academic issue. It's not.

All of these separate issues together are causing developers to seek other avenues. Some of them away from Tracing in general.

This is just the latest in a series of issues that are causing that. I've been raising and will continue to. Treating each one individually as a support ticket ignore the wealth of individual issues It's causing is how we lose the adoption race.

In terms of this library being treated as a Microsoft BCL library, look at the academic debate happening in this ticket and the other one. We could release 1.4 as is, we could then do a 1.5 or 2.0 with a rearchitecture. We could show the community that this OpenSource library can react fast to the demands of the customers, however, we're not.

We could have released a Hosting library as stable, we didn't, we bunched up into a large rewrite so we didn't need to do more releases. There's nothing stopping us doing more releases.

Like I say, this issue is highlighting that the governance is skewed towards Microsoft, and that is a concern for the community given all the recent issues with Microsoft and OpenSource.

[noahfalk]

In short, move this discussion of "Core" and "SDK" modularity ... what's an SDK into a 2.x release stream

Fwiw as the person who introduced a notion of 'core' into the discussion I fully agree it isn't a release blocking portion of the discussion and my apologies for diluting the issue. I raised it solely with the intent to say if OTel has a concern about satisfying different cohorts of users with different needs for dependencies in the long term, there are solutions to that problem.

I think it was fair feedback from @martinjt that as long as this issue remains marked release blocking we should constrain scope tightly. The starting scope was to eliminate the dependency on Configuration.EnvironmentVariables and I believe the code change to do it would be relatively simple and has no impact on external users. The quickest path forward appears to be: make that code change, mark this issue resolved, and move any remaining concerns about specific dependencies, re-introduction of the EnvironmentVariables dependency, or the overall philosophy on all dependencies into new non-release blocking issues.

[CodeBlanch]

Just FYI for everyone I have a couple draft PRs open for consuming the bits from EnvironmentVariables & the OptionsFactory from Options V5 (so we can revert back to 3.1): #4902 & #4093. Agree with @noahfalk we can always put back the dependencies if it makes sense to do so.