Skip to content
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

[api proposal] Cross-cutting UseOtlpExporter method #4940

Closed
CodeBlanch opened this issue Oct 11, 2023 · 4 comments · Fixed by #5400
Closed

[api proposal] Cross-cutting UseOtlpExporter method #4940

CodeBlanch opened this issue Oct 11, 2023 · 4 comments · Fixed by #5400
Assignees
Labels
enhancement New feature or request pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package
Milestone

Comments

@CodeBlanch
Copy link
Member

[This was requested by @davidfowl & @samsp-msft]

Goal

There is a lot of code required to configure traces, metrics, & logs to use OTLP at the same endpoint:

var useOtlpExporter = !string.IsNullOrWhiteSpace(builder.Configuration["OTEL_EXPORTER_OTLP_ENDPOINT"]);

if (useOtlpExporter)
{
    builder.Services.Configure<OpenTelemetryLoggerOptions>(logging => logging.AddOtlpExporter());
    builder.Services.ConfigureOpenTelemetryMeterProvider(metrics => metrics.AddOtlpExporter());
    builder.Services.ConfigureOpenTelemetryTracerProvider(tracing => tracing.AddOtlpExporter());
}

The goal is to turn this into a ~one-liner.

Proposal

OpenTelemetry (SDK)

namespace OpenTelemetry;

+public interface IOpenTelemetryBuilder
+{
+   IServiceCollection Services { get; }
+}

OpenTelemetry.Extensions.Hosting

namespace OpenTelemetry;

-public sealed class OpenTelemetryBuilder
+public sealed class OpenTelemetryBuilder : IOpenTelemetryBuilder
{
   // No other changes
}

OpenTelemetry.Exporter.OpenTelemetryProtocol

namespace OpenTelemetry;

+public sealed class OtlpExporterBuilderOptions
+{
+    public bool EnableLogging { get; set; } = true;
+    public bool EnableMetrics { get; set; } = true;
+    public bool EnableTracing { get; set; } = true;
+    public OtlpExporterOptions DefaultOptions { get; }
+    public OtlpExporterOptions LoggingOptions { get; }
+    public OtlpExporterOptions MetricsOptions { get; }
+    public OtlpExporterOptions TracingOptions { get; }
+}

+public static class OpenTelemetryBuilderOtlpExporterExtensions
+{
+    public static T AddOtlpExporter<T>(this T builder) where T : IOpenTelemetryBuilder {}
+    public static T AddOtlpExporter<T>(this T builder, Action<OtlpExporterBuilderOptions>? configure) where T : IOpenTelemetryBuilder {}
+    public static T AddOtlpExporter<T>(this T builder, string? name, Action<OtlpExporterBuilderOptions>? configure) where T : IOpenTelemetryBuilder {}
+}

Example implementation

https://github.com/open-telemetry/opentelemetry-dotnet/compare/main...CodeBlanch:poc/otlp-builder?expand=1

Scenarios enabled

  • The primary goal is accomplished:

    services.AddOpenTelemetry().AddOtlpExporter();

    That will turn on OTLP respecting the various environment variables.

  • And also really nice configuration integration is enabled:

    appSettings.json:

    {
       "opentelemetry_otlpexporter": {
          "DefaultOptions": {
              "Protocol": "HttpProtobuf"
          },
          "LoggingOptions": {
              "Endpoint": "http://log_endpoint/"
          },
          "MetricsOptions": {
              "Endpoint": "http://metric_endpoint/"
          },
          "TracingOptions": {
              "Endpoint": "http://trace_endpoint/"
          }
       }
    }

    bootstrap:

    services.Configure<OtlpExporterBuilderOptions>(appBuilder.Configuration.GetSection("opentelemetry_otlpexporter"));
    services.AddOpenTelemetry().AddOtlpExporter();
@CodeBlanch CodeBlanch added enhancement New feature or request pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package labels Oct 11, 2023
@davidfowl
Copy link
Contributor

There's a default endpoint you can set that gets applied to all of them right?

@CodeBlanch
Copy link
Member Author

CodeBlanch commented Oct 11, 2023

Yes you can set via OTEL_EXPORTER_OTLP_ENDPOINT or if using the json:

{
   "opentelemetry_otlpexporter": {
      "DefaultOptions": {
          "Protocol": "HttpProtobuf",
          "Endpoint": "http://otlp_endpoint/" // Note: We will append the signal path automatically to the default one
       }
   }
}

@cartermp
Copy link
Contributor

cartermp commented Nov 8, 2023

As an experimental extension this feels fine to build and then propose to the TC/spec group to see if we can get a "make it as easy as possible to configure all signals to go to the same spot" thing hashed out for all languages.

That said this really feels like code golfing to me. Collapsing 5 lines to 1 feels like it's a lot less of a problem being solved than critical spec deficiencies like this one: #2968

@martinjt
Copy link
Member

martinjt commented Nov 8, 2023

I think what we should be doing here is probably using the OTEL_EXPORTER_OTLP_ENDPOINT to configure it, rather than the code.

I could see something more that does a dynamic lookup to see if the OpenTelemetry.Exporter.OpenTelemetryProtocol package is present, and if the environment variable is set then use that and configure it for all signals. That makes it more consistent with the wider intiative around configuring from the outside.

I think the idea of "AddOpenTelemetry()` implicitly using config is a good one, then allowing people to configure either in code, explicitly by signal.

@utpilla utpilla added this to the 1.8.0 milestone Mar 9, 2024
@vishweshbankwar vishweshbankwar changed the title [api proposal] Cross-cutting AddOtlpExporter method [api proposal] Cross-cutting UseOtlpExporter method Mar 12, 2024
@CodeBlanch CodeBlanch self-assigned this Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants