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

ExporterOptions should not control the pipeline #2552

Closed
cijothomas opened this issue Oct 30, 2021 · 4 comments
Closed

ExporterOptions should not control the pipeline #2552

cijothomas opened this issue Oct 30, 2021 · 4 comments
Labels
enhancement New feature or request pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package Stale Issues and pull requests which have been flagged for closing due to inactivity

Comments

@cijothomas
Copy link
Member

Currently, the ExporterOptions( eg: OTLPExporterOptions etc), contains setting which are about the pipeline, but not about the exporter itself. (like whether to use BatchingProcessor or not, should use Metrics in delta vs cumulative mode etc). These are not really an Exporter setting, but they are now passed to Exporter ctor(). This is described in detail here, along with a possible mitigation. (since we are already stable for traces, we cannot break the API, so have to mitigate this only)
See : #2541 (comment)

@alanwest
Copy link
Member

alanwest commented Dec 1, 2021

Another idea… maybe there’s value in peeling apart the exporter options from the metric reader options - so the extension method might look like

public static MeterProviderBuilder AddOtlpExporter(
    this MeterProviderBuilder builder,
    Action<OtlpExporterOptions> configure = null,
    Action<OtlpMetricReaderOptions> configureMetricReader = null) { ... }

// The properties on this class currently exist on OtlpExporterOptions, but have not been released as stable
// So, they'd be removed from OtlpExporterOptions
public class OtlpMetricReaderOptions
{
    public MetricReaderType MetricReaderType { get; set; }
        = MetricReaderType.Periodic;
    public PeriodicExportingMetricReaderOptions PeriodicExportingMetricReaderOptions { get; set; }
        = new PeriodicExportingMetricReaderOptions();
    public AggregationTemporality AggregationTemporality { get; set; }
        = AggregationTemporality.Cumulative;
}

We could do the same thing with traces

public static TraceProviderBuilder AddOtlpExporter(
    this TraceProviderBuilder builder,
    Action<OtlpExporterOptions> configure = null,
    Action<OtlpSpanProcessorOptions> configureSpanProcessor = null) { ... }

// The properties on this class currently exist in the stable release of OtlpExporterOptions
// So, they'd be marked Obsolete there.
public class OtlpSpanProcessorOptions
{
    public ExportProcessorType ExportProcessorType { get; set; } = ExportProcessorType.Batch;
    public BatchExportProcessorOptions<Activity> BatchExportProcessorOptions { get; set; }
        = new BatchExportActivityProcessorOptions();
}

@CodeBlanch
Copy link
Member

[Talked about this on the SIG today, figured I would drop a comment here as well.]

RE: This design...

public static TraceProviderBuilder AddOtlpExporter(
    this TraceProviderBuilder builder,
    Action<OtlpExporterOptions> configure = null,
    Action<OtlpSpanProcessorOptions> configureSpanProcessor = null) { ... }

That works well but I think it has a couple of issues:

  • It is a bit intimidating for new users. Thinking most do not understand (or care) about processor/reader options, defaults are fine.
  • If you have other options classes (think serialization, transport, etc.) it might lead to a lot of delegates/overloads which is very confusing and hard to document.

For OneCollectorExporter I introduced a builder class OneCollectorLogExportProcessorBuilder for the extension to use.

It allows you to do things like this...

builder.AddOneCollectorExporter(configure => configure
   .SetConnectionString("InstrumentationKey=hello world")
   .ConfigureBatchOptions(o => o.ScheduledDelayMilliseconds = 1000)
   .ConfigureTransportOptions(o => o.Endpoint = new("http://localhost")));

Also plays nice with IConfiguration and Options API (all the bells and whistles) 🤣

Copy link
Contributor

github-actions bot commented Oct 8, 2024

This issue was marked stale due to lack of activity and will be closed in 7 days. Commenting will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Oct 8, 2024
@TimothyMothra
Copy link
Contributor

Spoke with Blanch and decided to close this.
This is related to #4527.
My understanding is that a fix is in place for Logs and Metrics. Traces is outstanding.

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 Issues related to OpenTelemetry NuGet package Stale Issues and pull requests which have been flagged for closing due to inactivity
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants