-
Notifications
You must be signed in to change notification settings - Fork 782
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
OTEL SPEC violation: OTEL_EXPORTER_OTLP_ENDPOINT
is not honored as an environment variable
#5586
Comments
Thanks for this report @julealgon!
There is default behavior which is to automatically load envvars into As a workaround, users who decide to take full control of envvar loading could do this right? builder.Configuration
.AddEnvironmentVariables("MyCustomPrefix_")
.AddEnvironmentVariables("OTEL_");
This seems problematic. I think the cure may be worse than the disease 🤣 |
I agree with you from one perspective, but disagree on others. First, keep in mind that the entire hosting model is optional. Not only that, but parts of the hosting model can be used separately in any process. For example, I could setup just configuration and dependency injection, using I think this goes against what the spec specifies: that this should be directly controllable by environment variables. Additionally, you can also create a cmdline tool with only the DI aspect and still leverage All I'm trying to say here is that we shouldn't force a higher level abstraction on people if they don't currently rely on it or for any reason can't rely on it. I discovered this in a couple of our projects because we are using a "blank" and partial host setup just to leverage configuration, logging and dependency injection without necessarily tapping into hosted services. Yes, its not the most common use case, but it is a valid one. As is simpler usages as I outlined above especially with cmdline tools that don't use generic host but still want some level of abstraction like configuration and DI. Similarly, you could be dealing with an old To be very clear here: this is currently not a blocker for us. We are feeding that endpoint setting via our global Azure AppConfiguration provider (and not via environment variables). I stumbled upon this while debugging one of the apps locally, putting the environment variable in the I thought that was a terrible-enough experience to warrant the creation of this issue for visibility, and because I trully believe that if you are not directly accepting an environment variable for this and relying on an abstraction, you are not compliying with the spec.
This is not a valid workaround. When you call
This would obviously be far from ideal as you just duplicated maintenance around those variables and they could eventually go out of sync.
The "with priority to IConfiguration" is just my immediate opinion but it could go either way. It would certainly need to be well docummented. I do not disagree it could be confusing, but I just don't see any other possibility for you to both honor the spec, and leverage the If the team decides to default the value to the environment variable, and then potentially override it with the value from |
Dang there's a lot to unpack 🤣 I want to talk about the The SDK doesn't require a host or Here are couple unit tests to try out: [Fact]
public void EnvVarTestUsingHostingExtensionsWithoutHost()
{
var expectedEndpoint = "http://my_custom_endpoint";
Environment.SetEnvironmentVariable("OTEL_EXPORTER_OTLP_ENDPOINT", expectedEndpoint);
var services = new ServiceCollection();
services.AddOpenTelemetry()
.WithTracing(tracing => tracing.AddOtlpExporter());
using var sp = services.BuildServiceProvider();
sp.GetRequiredService<TracerProvider>();
Assert.Equal(expectedEndpoint, sp.GetRequiredService<IConfiguration>()["OTEL_EXPORTER_OTLP_ENDPOINT"]);
var options = sp.GetRequiredService<IOptions<OtlpExporterOptions>>().Value;
Assert.Equal(expectedEndpoint, options.Endpoint.OriginalString);
}
[Fact]
public void EnvVarTestUsingSdkCreateStyle()
{
var testRun = false;
var expectedEndpoint = "http://my_custom_endpoint";
Environment.SetEnvironmentVariable("OTEL_EXPORTER_OTLP_ENDPOINT", expectedEndpoint);
using var tracerProvider = Sdk.CreateTracerProviderBuilder()
.AddOtlpExporter()
.ConfigureServices(services =>
{
services.ConfigureOpenTelemetryTracerProvider((sp, _) =>
{
var options = sp.GetRequiredService<IOptions<OtlpExporterOptions>>().Value;
Assert.Equal(expectedEndpoint, options.Endpoint.OriginalString);
Assert.Equal(expectedEndpoint, sp.GetRequiredService<IConfiguration>()["OTEL_EXPORTER_OTLP_ENDPOINT"]);
testRun = true;
});
})
.Build();
Assert.True(testRun);
} In both cases there is no host or How? Lines 66 to 72 in eace49c
The SDK will create So for non-hosting scenarios, I don't think the issue applies. Users will just get envvars in that case and the SDK will behave as the spec defines. For hosting scenarios using the defaults, everything will also work as the spec defines. The only case where things will get strange is if users decide to manually construct their For sure the spec doesn't talk about I'm not saying I wouldn't like to fix this, I just don't see a way at the moment to make it perfect. Open to ideas! |
Sorry about that 😅
This is insteresting. So in a sense, the fallback chain I mentioned already exists here, but goes the other way: the envvars are a fallback to lack of
Exactly. And I think this might be a more common scenario than you are alluding to. If I built a console app for example and didn't want to go all the way with generic host + hosted services, creating my own
You might have a point here, but that's just not how this works usually. Following your logic, if I didn't load envvars wouldn't I want my entire application to be shielded from them? That's just not what happens: they keep working normally and completely disregard Take something like the Azure credential variables. I think these OTEL envvars should behave like that. They should work regardless of
Sure, but what I'm proposing would still leverage
I will reiterate this: this is not a problem for us specifically, but I think it is a consistency/principle of least astonishment violation for sure. I think the scenarios where the envvars are ignored today are incredibly confusing, in particular when people refer to general OpenTelemetry documentation and see that they should work. I think I presented enough cases to support my point but I'll leave this up to you of course. I still believe it is a mistake to flat out ignore the environment variables in some cases. They should, at least, always be the default value and overriden by higher level abstractions then. I believe the approach this library takes, of creating an |
I wasted an hour of my time trying to understand why my OTLP Collector I don't think there's any value-add to making this an |
* install OpenTelemetry plugins * added OpenTelemetry * trying to figure out OTEL resource conventions * added assembly version data * fixed DLL hell issues * simplify OTEL exporter config * fixed K8s OTEL config for pod * fixed set * Manually load `OTEL_EXPORTER_OTLP_ENDPOINT` issue work around for open-telemetry/opentelemetry-dotnet#5586
Now now... that's is just crazy. We rely on If only environment variables were supported, this would be an absolute mess to manage as we'd have to set that variable individually in several places. It needs to support both. |
My comment should be read as: only supporting Msft dependency injection is crazy. |
Decided to check some of the other popular environment variables while I was at it: services
.AddOpenTelemetry()
.ConfigureResource(builder =>
{
builder
.AddEnvironmentVariableDetector() // need this
.AddTelemetrySdk()
.AddAttributes(new []
{
new KeyValuePair<string, object>("service.version", typeof(OpenTelemetryConfig).Assembly.GetName().Version?.ToString() ?? "unknown")
});
}) - name: OTEL_RESOURCE_ATTRIBUTES
value: "service.namespace=$(NAMESPACE),service.instance.id=$(POD_NAME)"
- name: OTEL_SERVICE_NAME
value: "drawtogether" I see all of that data show up in Aspire / Grafana et al, so that works so long as the |
|
I'm not following you at all here. Care to elaborate? |
Something in your report isn't adding up for me. The issue @julealgon reported is when calling Host.CreateEmptyApplicationBuilder then environment variables are NOT automatically loaded. But you are calling
(PS: You have code doing the envvar load and loading
I don't think it is a "footgun" because if it was, people would be screaming at us 🤣 Lots of users are successfully doing what you are trying to do. And if you use the default patterns, everything works fine automatically. Only if you go out of your way to ask for an "empty" host do you then have to take some action.
Everything worked fine! I'm guessing the issue is/was in your k8s config loading somewhere. Not super familiar with k8s, did you make sure all updated config was deployed when you were testing? |
Yes, numerous times - it never worked until I added the work-around. |
i.e., I was inspecting the |
The relevant K8s configuration is OSS too: Edit: worth mentioning: I didn't use OTEL Collector |
Here's some code you can use to inspect things with debugger: using Microsoft.Extensions.Options;
using OpenTelemetry;
// Same test can be performed using real vars on the system
Environment.SetEnvironmentVariable("OTEL_EXPORTER_OTLP_ENDPOINT", "http://helloworld/");
var builder = WebApplication.CreateBuilder(args);
builder.Services.AddOpenTelemetry().UseOtlpExporter();
var app = builder.Build();
var optionsType = typeof(OpenTelemetryBuilderOtlpExporterExtensions)
.Assembly
.GetType("OpenTelemetry.Exporter.OtlpExporterBuilderOptions");
var options = app.Services.GetService(typeof(IOptions<>).MakeGenericType(optionsType));
app.Run(); It is picking up the envvar: So as far as I can tell, everything seems to be working fine. |
per @CodeBlanch 's comment, I'm not correct on that - so disregard |
As a point for always dealing with the env variable directly: the variables to control aspnetcore is not affected by this, for example the variables to control which ports should be bound. |
I would expect them to apply because the spec says they do. I want components like the OTEL SDK to work consistently according to their own rules. I don't want them to guess my intentions based on how I use other components. Reading environment variables directly means there's a consistent way to configure OTEL components regardless of language or platform. It's also a documentation problem: opentelemetry-dotnet/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs Lines 20 to 24 in 22a58aa
That's not strictly true. The |
@CodeBlanch I encountered the same issue as @Aaronontheweb and the workaround is the only thing that made this work. |
Bug Report
List of all OpenTelemetry NuGet
packages and version that you are
using (e.g.
OpenTelemetry 1.0.2
):Tested with:
OpenTelemetry.Exporter.OpenTelemetryProtocol 1.8.0
OpenTelemetry.Exporter.OpenTelemetryProtocol 1.8.1
Runtime version (e.g.
net462
,net48
,netcoreapp3.1
,net6.0
etc. You canfind this information from the
*.csproj
file):net472
net8.0
Symptom
When using either
UseOtlpExporter
orAddOtlpExporter
to configure the OTLP exporter, theOTEL_EXPORTER_OTLP_ENDPOINT
is not respected when provided as an environment variable. The setting is only honored if included inIConfiguration
, which is not always the case in every project.What is the expected behavior?
I would expect the environment variable to be honored regardless of the
Configuration
setup in the application: the setting should be read both from environment variables directly as well as fromIConfiguration
, so it respects both the spec (which only talks about environment variables) as well as the standard .NET configuration ecosystem.What is the actual behavior?
The setting is only respected if it is found in
IConfiguration
. If for whatever reason the current project did not use.AddEnvironmentVariables
without a prefix in the host setup, the setting is ignored completely.Reproduce
https://github.com/julealgon/OpenTelemetry.UseOtlpExporterIssue
OpenTelemetry.UseOtlpExporterIssue
solutionlaunchsettings.json
and observe the differenceAdditional Context
This inconsistency appears to have been introduced between versions 1.3 and 1.4 of the package. Downgrading to 1.3 respects the environment variable, but stops reading it from
IConfiguration
.Both options should work, with priority to
IConfiguration
.The text was updated successfully, but these errors were encountered: