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

[Logs] Sync up logger provider build-up scenarios with tracer provider #3596

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Aug 22, 2022

Changes

This PR syncs up the LoggerProvider scenarios (added on #3504) with the TracerProvider scenarios (#3533)

  • AddExporter methods added
  • AddOpenTelemetry is now safe to be called multiple times
  • Removed the ability to register BaseProcessor<LogRecord>s & Action<IServiceProvider, OpenTelemetryLoggerProvider>s into the IServiceCollection

Public API

namespace OpenTelemetry.Logs
{
   public class OpenTelemetryLoggerOptions
   {
+     public OpenTelemetryLoggerOptions AddExporter(ExportProcessorType exportProcessorType, BaseExporter<LogRecord> exporter) {}
+     public OpenTelemetryLoggerOptions AddExporter(ExportProcessorType exportProcessorType, BaseExporter<LogRecord> exporter, Action<ExportLogRecordProcessorOptions> configure) {}
+     public OpenTelemetryLoggerOptions AddExporter<T>(ExportProcessorType exportProcessorType) where T : BaseExporter<LogRecord> {}
+     public OpenTelemetryLoggerOptions AddExporter<T>(ExportProcessorType exportProcessorType, Action<ExportLogRecordProcessorOptions> configure) where T : BaseExporter<LogRecord> {}
   }

    // This class is similar to MetricReaderOptions and is used by AddExporter extensions
+   public class ExportLogRecordProcessorOptions
+   {
+      public ExportProcessorType ExportProcessorType { get; set; }
+      public BatchExportLogRecordProcessorOptions BatchExportProcessorOptions { get; set; }
+   }

    // This class is similar to BatchExportActivityProcessorOptions and exists so separate environment variables may be used to configure batch log processor settings
+   public class BatchExportLogRecordProcessorOptions
+   {
+   }
}

TODOs

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Changes in public API reviewed
  • Unit tests

@codecov
Copy link

codecov bot commented Aug 22, 2022

Codecov Report

Merging #3596 (da15761) into main (9c3d1b1) will increase coverage by 0.20%.
The diff coverage is 91.22%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3596      +/-   ##
==========================================
+ Coverage   87.14%   87.34%   +0.20%     
==========================================
  Files         280      282       +2     
  Lines       10112    10132      +20     
==========================================
+ Hits         8812     8850      +38     
+ Misses       1300     1282      -18     
Impacted Files Coverage Δ
...enTelemetry/Logs/OpenTelemetryLoggingExtensions.cs 100.00% <ø> (+2.77%) ⬆️
...metry/Logs/BatchExportLogRecordProcessorOptions.cs 60.00% <60.00%> (ø)
...lemetry/Logs/Options/OpenTelemetryLoggerOptions.cs 99.01% <97.56%> (-0.99%) ⬇️
...nTelemetry/Logs/ExportLogRecordProcessorOptions.cs 100.00% <100.00%> (ø)
.../OpenTelemetry/Logs/OpenTelemetryLoggerProvider.cs 93.82% <100.00%> (-0.80%) ⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 79.41% <0.00%> (-2.95%) ⬇️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs 97.65% <0.00%> (+0.78%) ⬆️
src/OpenTelemetry/Logs/OpenTelemetryLogger.cs 88.88% <0.00%> (+2.22%) ⬆️
...metryProtocol/Implementation/ActivityExtensions.cs 95.05% <0.00%> (+3.29%) ⬆️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs 77.27% <0.00%> (+40.90%) ⬆️
... and 1 more

@CodeBlanch CodeBlanch marked this pull request as ready for review August 29, 2022 20:19
@CodeBlanch CodeBlanch requested a review from a team August 29, 2022 20:19
@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2022

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing 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 Sep 6, 2022
@github-actions github-actions bot removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Sep 7, 2022
/// </remarks>
public class BatchExportLogRecordProcessorOptions : BatchExportProcessorOptions<LogRecord>
{
internal const string MaxQueueSizeEnvVarKey = "OTEL_DOTNET_BLP_MAX_QUEUE_SIZE";
Copy link
Member

Choose a reason for hiding this comment

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

I assume the spec will declare official environment variable names. You think we can be optimistic and remove DOTNET from these environment variable names and just go with OTEL_BLP_MAX_QUEUE_SIZE for example?

Copy link
Member

Choose a reason for hiding this comment

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

oh scratch that i just saw the previous resolved conversation..

Copy link
Member

@alanwest alanwest left a comment

Choose a reason for hiding this comment

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

LGTM

@CodeBlanch CodeBlanch merged commit 5208ec0 into open-telemetry:main Sep 7, 2022
@CodeBlanch CodeBlanch deleted the loggerprovider-dependencyinjection-2 branch September 7, 2022 21:22
@CodeBlanch
Copy link
Member Author

Merged to keep moving. Hoping to settle the environment variables before stable release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants