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

Conversation

CodeBlanch
Copy link
Member

[Originally part of #5400]

Changes

  • Adds ActivityExportProcessorOptions for tracing. This brings tracing in line with logging (LogRecordExportProcessorOptions) and metrics (MetricReaderOptions).
  • Fixes options registrations so that all three signals work the same way:
    • Logging users can access\bind LogRecordExportProcessorOptions (already in place) or BatchExportLogRecordProcessorOptions (already in place)
    • Metrics users can access\bind MetricReaderOptions (already in place) or PeriodicExportingMetricReaderOptions (added on this pr)
    • Tracing users can access\bind ActivityExportProcessorOptions (added on this pr) or BatchExportActivityProcessorOptions (already in place)

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@CodeBlanch CodeBlanch added pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package metrics Metrics signal related logs Logging signal related traces Tracing signal related labels Mar 7, 2024
@CodeBlanch CodeBlanch requested a review from a team March 7, 2024 22:08
Copy link

codecov bot commented Mar 7, 2024

Codecov Report

Attention: Patch coverage is 40.90909% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 84.61%. Comparing base (6250307) to head (397c17d).
Report is 118 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5423      +/-   ##
==========================================
+ Coverage   83.38%   84.61%   +1.23%     
==========================================
  Files         297      282      -15     
  Lines       12531    12116     -415     
==========================================
- Hits        10449    10252     -197     
+ Misses       2082     1864     -218     
Flag Coverage Δ
unittests ?
unittests-Solution-Experimental 84.61% <40.90%> (?)
unittests-Solution-Stable 84.34% <40.90%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/OpenTelemetry/Metrics/MetricReaderOptions.cs 100.00% <100.00%> (ø)
...lder/ProviderBuilderServiceCollectionExtensions.cs 91.30% <71.42%> (-8.70%) ⬇️
...nTelemetry/Trace/ActivityExportProcessorOptions.cs 0.00% <0.00%> (ø)

... and 55 files with indirect coverage changes

/// <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.

@CodeBlanch CodeBlanch merged commit 0820101 into open-telemetry:main Mar 7, 2024
37 checks passed
@CodeBlanch CodeBlanch deleted the sdk-options-cleanup branch March 7, 2024 22:33
@CodeBlanch
Copy link
Member Author

Just in case anyone is wondering...

  • There wasn't a CHANGELOG update on this PR. The reason for that is ActivityExportProcessorOptions can't be used for much right now. @alanwest and I decided we should do follow-up PRs to use ActivityExportProcessorOptions with OTLP & Zipkin and then we can add the entry.

  • There aren't tests on this PR. We don't have direct tests for LogRecordExportProcessorOptions either. It is only tested via OTLP tests. So I'll add similar tests for ActivityExportProcessorOptions on [otlp] UseOtlpExporter cross-cutting extension #5400

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logs Logging signal related metrics Metrics signal related pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package traces Tracing signal related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants