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

[sdk] Add ActivityExportProcessorOptions and clean up MetricReaderOptions #5423

Merged
merged 1 commit into from
Mar 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/OpenTelemetry/.publicApi/Stable/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
OpenTelemetry.OpenTelemetryBuilderSdkExtensions
OpenTelemetry.Trace.ActivityExportProcessorOptions
OpenTelemetry.Trace.ActivityExportProcessorOptions.ActivityExportProcessorOptions() -> void
OpenTelemetry.Trace.ActivityExportProcessorOptions.BatchExportProcessorOptions.get -> OpenTelemetry.Trace.BatchExportActivityProcessorOptions!
OpenTelemetry.Trace.ActivityExportProcessorOptions.BatchExportProcessorOptions.set -> void
OpenTelemetry.Trace.ActivityExportProcessorOptions.ExportProcessorType.get -> OpenTelemetry.ExportProcessorType
OpenTelemetry.Trace.ActivityExportProcessorOptions.ExportProcessorType.set -> void
static OpenTelemetry.OpenTelemetryBuilderSdkExtensions.ConfigureResource(this OpenTelemetry.IOpenTelemetryBuilder! builder, System.Action<OpenTelemetry.Resources.ResourceBuilder!>! configure) -> OpenTelemetry.IOpenTelemetryBuilder!
static OpenTelemetry.OpenTelemetryBuilderSdkExtensions.WithMetrics(this OpenTelemetry.IOpenTelemetryBuilder! builder) -> OpenTelemetry.IOpenTelemetryBuilder!
static OpenTelemetry.OpenTelemetryBuilderSdkExtensions.WithMetrics(this OpenTelemetry.IOpenTelemetryBuilder! builder, System.Action<OpenTelemetry.Metrics.MeterProviderBuilder!>! configure) -> OpenTelemetry.IOpenTelemetryBuilder!
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,6 @@ public static IServiceCollection AddOpenTelemetryLoggerProviderBuilderServices(t

services!.TryAddSingleton<LoggerProviderBuilderSdk>();
services!.RegisterOptionsFactory(configuration => new BatchExportLogRecordProcessorOptions(configuration));

// Note: This registers a factory so that when
// sp.GetRequiredService<IOptionsMonitor<LogRecordExportProcessorOptions>>().Get(name)))
// is executed the SDK internal
// BatchExportLogRecordProcessorOptions(IConfiguration) ctor is used
// correctly which allows users to control the OTEL_BLRP_* keys using
// IConfiguration (envvars, appSettings, cli, etc.).
services!.RegisterOptionsFactory(
(sp, configuration, name) => new LogRecordExportProcessorOptions(
sp.GetRequiredService<IOptionsMonitor<BatchExportLogRecordProcessorOptions>>().Get(name)));
Expand All @@ -40,7 +33,10 @@ public static IServiceCollection AddOpenTelemetryMeterProviderBuilderServices(th
Debug.Assert(services != null, "services was null");

services!.TryAddSingleton<MeterProviderBuilderSdk>();
services!.RegisterOptionsFactory(configuration => new MetricReaderOptions(configuration));
services!.RegisterOptionsFactory(configuration => new PeriodicExportingMetricReaderOptions(configuration));
services!.RegisterOptionsFactory(
(sp, configuration, name) => new MetricReaderOptions(
sp.GetRequiredService<IOptionsMonitor<PeriodicExportingMetricReaderOptions>>().Get(name)));

return services!;
}
Expand All @@ -51,6 +47,9 @@ public static IServiceCollection AddOpenTelemetryTracerProviderBuilderServices(t

services!.TryAddSingleton<TracerProviderBuilderSdk>();
services!.RegisterOptionsFactory(configuration => new BatchExportActivityProcessorOptions(configuration));
services!.RegisterOptionsFactory(
(sp, configuration, name) => new ActivityExportProcessorOptions(
sp.GetRequiredService<IOptionsMonitor<BatchExportActivityProcessorOptions>>().Get(name)));

return services!;
}
Expand Down
11 changes: 7 additions & 4 deletions src/OpenTelemetry/Metrics/MetricReaderOptions.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

using Microsoft.Extensions.Configuration;
using System.Diagnostics;
using OpenTelemetry.Internal;

namespace OpenTelemetry.Metrics;
Expand All @@ -17,13 +17,16 @@ public class MetricReaderOptions
/// Initializes a new instance of the <see cref="MetricReaderOptions"/> class.
/// </summary>
public MetricReaderOptions()
: this(new ConfigurationBuilder().AddEnvironmentVariables().Build())
: this(new())
{
}

internal MetricReaderOptions(IConfiguration configuration)
internal MetricReaderOptions(
PeriodicExportingMetricReaderOptions defaultPeriodicExportingMetricReaderOptions)
{
this.periodicExportingMetricReaderOptions = new PeriodicExportingMetricReaderOptions(configuration);
Debug.Assert(defaultPeriodicExportingMetricReaderOptions != null, "defaultPeriodicExportingMetricReaderOptions was null");

this.periodicExportingMetricReaderOptions = defaultPeriodicExportingMetricReaderOptions ?? new();
}

/// <summary>
Expand Down
49 changes: 49 additions & 0 deletions src/OpenTelemetry/Trace/ActivityExportProcessorOptions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

using System.Diagnostics;
using OpenTelemetry.Internal;

namespace OpenTelemetry.Trace;

/// <summary>
/// Options for configuring either a <see cref="SimpleActivityExportProcessor"/> or <see cref="BatchActivityExportProcessor"/>.
/// </summary>
public class ActivityExportProcessorOptions
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add this now?

Today we have these options essentially exposed directly on OtlpExporterOptions:

/// <summary>
/// Gets or sets the export processor type to be used with the OpenTelemetry Protocol Exporter. The default value is <see cref="ExportProcessorType.Batch"/>.
/// </summary>
/// <remarks>Note: This only applies when exporting traces.</remarks>
public ExportProcessorType ExportProcessorType { get; set; } = ExportProcessorType.Batch;
/// <summary>
/// Gets or sets the BatchExportProcessor options. Ignored unless ExportProcessorType is Batch.
/// </summary>
/// <remarks>Note: This only applies when exporting traces.</remarks>
public BatchExportProcessorOptions<Activity> BatchExportProcessorOptions { get; set; }

Originally that made sense when we only had tracing. But now we have logging and metrics which also use OtlpExporterOptions but need different settings. For metrics we have MetricReaderOptions. For logging we have LogRecordExportProcessorOptions.

On #5400 we're adding a new UseOtlpExporter method. Instead of bringing forward this mess that extension will use ActivityExportProcessorOptions so it mirrors logging & metrics and hides away these two options which really should have never been exposed.

/cc @alanwest

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should mark the old options obsolete.

{
private BatchExportActivityProcessorOptions batchExportProcessorOptions;

/// <summary>
/// Initializes a new instance of the <see cref="ActivityExportProcessorOptions"/> class.
/// </summary>
public ActivityExportProcessorOptions()
: this(new())
{
}

internal ActivityExportProcessorOptions(
BatchExportActivityProcessorOptions defaultBatchExportActivityProcessorOptions)
{
Debug.Assert(defaultBatchExportActivityProcessorOptions != null, "defaultBatchExportActivityProcessorOptions was null");

this.batchExportProcessorOptions = defaultBatchExportActivityProcessorOptions ?? new();
}

/// <summary>
/// Gets or sets the export processor type to be used. The default value is <see cref="ExportProcessorType.Batch"/>.
/// </summary>
public ExportProcessorType ExportProcessorType { get; set; } = ExportProcessorType.Batch;

/// <summary>
/// Gets or sets the batch export options. Ignored unless <see cref="ExportProcessorType"/> is <see cref="ExportProcessorType.Batch"/>.
/// </summary>
public BatchExportActivityProcessorOptions BatchExportProcessorOptions
{
get => this.batchExportProcessorOptions;
set
{
Guard.ThrowIfNull(value);
this.batchExportProcessorOptions = value;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,10 @@ public void CreatePeriodicExportingMetricReader_FromIConfiguration()
.AddInMemoryCollection(values)
.Build();

var options = new MetricReaderOptions(configuration);
var options = new PeriodicExportingMetricReaderOptions(configuration);

Assert.Equal(18, options.PeriodicExportingMetricReaderOptions.ExportIntervalMilliseconds);
Assert.Equal(19, options.PeriodicExportingMetricReaderOptions.ExportTimeoutMilliseconds);
Assert.Equal(18, options.ExportIntervalMilliseconds);
Assert.Equal(19, options.ExportTimeoutMilliseconds);
}

[Fact]
Expand Down