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