Added OpenTelemetryEnvironment

[AlbertoMonteiro]

This method configure with default environmental variables definitions

Fixes #12

[davidfowl]

Is there a test?

[AlbertoMonteiro]

Is there a test?

I will add later

[nblumhardt]

Thanks for kicking this off, @AlbertoMonteiro!

A few initial thoughts -

• The method you've added targets LoggerAuditSinkConfiguration; for this use case, it's LoggerSinkConfiguration that should be targeted
• To get started, I don't think the new configuration overload should accept any parameters at all - just WriteTo.OpenTelemetry() should either read everything from the environment, or use the (OTel-defined) defaults for anything else
• There are some subtleties in the spec around how the endpoint is interpreted; in our use case I think this will mean appending /v1/logs if the endpoint doesn't already carry that suffix; there are also _LOGS_ versions of the environment variables that should override the generic endpoint address if present
• Reading of the environment variables (and constructing resource attributes/headers) might be best factored out into an OpenTelemetryEnvironmentVariables kind of helper class, in part to keep the configuration extension method type clean, and also to facilitate testing

Super keen to see this work smoothly, let me know if I can clarify any of the above, or unblock anything that holds up the works :-)

[AlbertoMonteiro]

Hey @nblumhardt tyvm for your review.

• I've added the OpenTelemetryEnvironmentVariables, let me know if you if you think that I should move it to some other folder/namespace
• Added tests to OpenTelemetryEnvironmentVariables
• I fixed the method, and now it targets the LoggerSinkConfiguration and calls the OpenTelemetry method sending proper arguments. I just keep the log level arguments, since I didn't find any env var from OTEL that I can get this information.

I also would like to know the best way to test the OpenTelemetryEnvironment method, since I didn't find unit tests for the other methods.

I also didn't understand entirely the stuff around the path /v1/logs. The code I've written worked fine with .NET Aspire, I got this code from @davidfowl in another issue (serilog/serilog-aspnetcore#359 (comment))

[nblumhardt]

Thanks for the updates @AlbertoMonteiro!

I need to give a little more thought to how this should fit in with the existing WriteTo.OpenTelemetry() methods before digging into all of the implementation details, just down to trivia otherwise.

For example, we could avoid the need for an additional configuration method if the existing configuration methods supported an includeEnvironment option:

    public static LoggerConfiguration OpenTelemetry(
        this LoggerSinkConfiguration loggerSinkConfiguration,
        string endpoint = OpenTelemetrySinkOptions.DefaultEndpoint,
        OtlpProtocol protocol = OpenTelemetrySinkOptions.DefaultProtocol,
        IDictionary<string, string>? headers = null,
        IDictionary<string, object>? resourceAttributes = null,
        IncludedData? includedData = null,
        LogEventLevel restrictedToMinimumLevel = LevelAlias.Minimum,
-        LoggingLevelSwitch? levelSwitch = null)
+        LoggingLevelSwitch? levelSwitch = null,
+        bool includeEnvironment = false)
    
    public static LoggerConfiguration OpenTelemetry(
        this LoggerSinkConfiguration loggerSinkConfiguration,
-        Action<OpenTelemetrySinkOptions> configure)
+        Action<OpenTelemetrySinkOptions> configure,
+        bool includeEnvironment = false)

(The default might end up being true FWIW, just sketching..)

Then, where the sink is configured, OpenTelemetryEnvironment would be given an opportunity to override the options that were otherwise supplied through code:

    public static LoggerConfiguration OpenTelemetry(
        this LoggerSinkConfiguration loggerSinkConfiguration,
-        Action<OpenTelemetrySinkOptions> configure)
+        Action<OpenTelemetrySinkOptions> configure,
+        bool includeEnvironment = false)
    {
        if (configure == null) throw new ArgumentNullException(nameof(configure));
        var options = new OpenTelemetrySinkOptions();
        
        configure(options);
+        if (includeEnvironment)
+        {
+            OpenTelemetryEnvironment.Configure(options);
+        }

        var exporter = Exporter.Create(

We could also then aim to line up the defaults specified for the existing configuration method with the (I assume) OpenTelemetry-specified defaults.

Any thoughts?

[AlbertoMonteiro]

@nblumhardt thanks for the detailed answer.

I like the idea of using the parameter to use env var values.

So I just added a new commit with that change.

[nblumhardt]

Awesome! Thanks for making the change. I've run through and reviewed line-by-line, if any of the comments are unclear please just let me know.

[nblumhardt]

These should only be applied when the environment variables are present. When no environment variables are set, the configure method shouldn't do anything.

[nblumhardt]

Support for OTEL_EXPORTER_OTLP_PROTOCOL will be needed in order for this to work with both supported protocols.

[nblumhardt]

When OTEL_EXPORTER_OTLP_ENDPOINT is provided, and the protocol is determined to be http/protobuf, if the value doesn't end with /v1/logs it will be necessary to tack this on. (In a near-future version of the sink this will be fixed, but some more pieces need to come together around it.)

[nblumhardt]

Nit: exception messages should end with ., although it's a bit awkward here.

[AlbertoMonteiro]

I've added the header name to make debugging easier.

About the exception here, since the value can come from manual creation, I think that being able to know when the value is not in the expected format can help to fix it asap.

But if you still think that it isn't necessary here, I can remove it.

[nblumhardt]

Using the environment variables directly will cause issues if the tests rely on OTLP logging at some point in the future, and will cause tests to interfere with each other when running concurrently.

Perhaps OpenTelemetryEnvironment could have an internal Configure(options, endpointVariable, headersVariable, resourceAttributesVariable) method added, so that test can call it without setting the actual environment variables?

The "real" configure method could then just be a simple, untested:

    public static void Configure(BatchedOpenTelemetrySinkOptions options)
    {
        Configure(
            options,
            Environment.GetEnvironmentVariable("OTEL_EXPORTER_OTLP_ENDPOINT"),
            Environment.SetEnvironmentVariable("OTEL_EXPORTER_OTLP_HEADERS"), ...

?

[AlbertoMonteiro]

Thanks for the advice!

I've fixed it in another way, the Configure method receives the function that can get the env var value, so in the test, I can provide a mock function.

[AlbertoMonteiro]

Hey @nblumhardt I've pushed the fixes!

Let me know if still need some changes

[nblumhardt]

Looks great, thank you @AlbertoMonteiro 🎉

[nblumhardt]

I'll update the version of the package to 4.0.0-dev-*, since the added parameter is a binary breaking change (and reading variables by default is a breaking behavioral one).

[AlbertoMonteiro]

Thanks for your support @nblumhardt !

[theperm]

I think this PR is welcome but has missed the mark a little. It only caters for the default OTEL env vars and misses the signal specific ones for logs. These are

OTEL_EXPORTER_OTLP_LOGS_ENDPOINT="http://localhost:5341/ingest/otlp/v1/logs"
OTEL_EXPORTER_OTLP_LOGS_PROTOCOL=http/protobuf
OTEL_EXPORTER_OTLP_LOGS_HEADERS="xxxxxx"

If i want my metrics and traces to go to the defaults but i want the logs to go to Seq I can not do this at the LOG level.
I will have to override both metrics and traces signal env vars and have lofs use the defaults - this would be undesirable.