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 + otlp] Add log processor factory overload #4916

Merged
merged 37 commits into from
Nov 22, 2023

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Oct 3, 2023

Relates to #4892
Fixes #4259

Changes

  • Adds an AddProcessor overload on OpenTelemetryLoggerOptions which exposes the factory pattern (Func<IServiceProvider, BaseProcessor<LogRecord>> implementationFactory).

  • Uses the factory overload in OTLP log exporter registration to support options being configured through IConfiguration.

  • Adds an OTLP log exporter registration overload accepting string name to support named options being configured through IConfiguration.

Details

The goal of this work is to close some of the gap between logs & traces/metrics. Currently in traces & metrics users may modify the IServiceCollection behind the builders and then interact with the final IServiceProvider during startup. This PR adds half of that support to logs: interacting with the final IServiceProvider during startup. This is useful for OTLP because it can now retrieve IConfiguration and IOptions instances.

A couple features which will be enabled with this PR:

  • Setting sdk limits & experimental options through IConfiguration.

  • Configuring exporter settings through Options API, eg:

    services.Configure<OtlpExporterOptions>(o => o.Endpoint= new Uri(...));
    // or
    services.Configure<OtlpExporterOptions>(config.GetSection("opentelemetry:otlp"));
  • Configuring batch settings through Options API, eg:

    services.Configure<BatchExportLogRecordProcessorOptions>(o => o.ScheduledDelayMilliseconds = 5000);
    // or
    services.Configure<BatchExportLogRecordProcessorOptions>(config.GetSection("opentelemetry:logs:batchoptions"));

 

Merge requirement checklist

  • CONTRIBUTING guidelines followed (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 pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package labels Oct 3, 2023
@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Merging #4916 (3477837) into main (f9c300b) will decrease coverage by 0.14%.
The diff coverage is 93.47%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4916      +/-   ##
==========================================
- Coverage   83.40%   83.27%   -0.14%     
==========================================
  Files         296      296              
  Lines       12301    12357      +56     
==========================================
+ Hits        10260    10290      +30     
- Misses       2041     2067      +26     
Flag Coverage Δ
unittests 83.27% <93.47%> (-0.14%) ⬇️

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

Files Coverage Δ
...orter.OpenTelemetryProtocol/OtlpExporterOptions.cs 96.36% <100.00%> (ø)
...nTelemetryProtocol/OtlpMetricExporterExtensions.cs 99.00% <100.00%> (+0.14%) ⬆️
...metryProtocol/OtlpTraceExporterHelperExtensions.cs 96.66% <ø> (ø)
...lemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs 86.95% <100.00%> (+1.95%) ⬆️
...emetry/Logs/ILogger/OpenTelemetryLoggerProvider.cs 93.54% <100.00%> (+1.61%) ⬆️
...try/Logs/ILogger/OpenTelemetryLoggingExtensions.cs 94.73% <100.00%> (+0.14%) ⬆️
src/OpenTelemetry/ProviderExtensions.cs 90.47% <ø> (ø)
....Exporter.OpenTelemetryProtocol/OtlpLogExporter.cs 88.88% <85.71%> (-5.40%) ⬇️
...lemetryProtocol/OtlpLogExporterHelperExtensions.cs 89.83% <89.58%> (-0.65%) ⬇️

... and 10 files with indirect coverage changes


var name = Options.DefaultName;

return loggerOptions.AddProcessor(sp =>
Copy link
Member

Choose a reason for hiding this comment

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

From end user perspective: This scenario of enabling IConfiguration/SdkLimitOptions/BatchOptions will only be possible when they have the servicecollection already available like in ASP.NET Core apps. Unlike in case of tracing/metrics where they can call ConfigureServices extension. Is my understanding correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Typically I would expect only host users (AspNetCore or generic host) to care about deep IConfiguration integration. But it is also available for manual scenarios too if users want it.

Here is how you would do it manually for traces & logs:

var config = new ConfigurationBuilder()
    .AddJsonFile("appSettings.json")
    .AddEnvironmentVariables()
    .AddCommandLine(args)
    .Build();

using var tracerProvider = Sdk.CreateTracerProviderBuilder()
    .ConfigureServices(services => services.AddSingleton<IConfiguration>(config))
    .AddOtlpExporter()
    .Build();

using var loggerFactory = LoggerFactory.Create(builder =>
{
    builder.Services.AddSingleton<IConfiguration>(config);

    builder.AddOpenTelemetry(logging => logging.AddOtlpExporter());
});

Copy link
Member

Choose a reason for hiding this comment

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

Could we add a test to validate this scenario?

Copy link
Contributor

Choose a reason for hiding this comment

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

see:

public void VerifyEnvironmentVariablesTakenFromIConfigurationWhenUsingLoggerFactoryCreate(string name, bool optionalNameMatch)

@github-actions
Copy link
Contributor

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 label Oct 12, 2023
@github-actions
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Oct 19, 2023
@github-actions github-actions bot removed the Stale label Nov 9, 2023
@CodeBlanch CodeBlanch marked this pull request as ready for review November 9, 2023 22:18
@CodeBlanch CodeBlanch requested a review from a team as a code owner November 9, 2023 22:18
Copy link
Member

@vishweshbankwar vishweshbankwar left a comment

Choose a reason for hiding this comment

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

Looks good - Needs test #4916 (comment)

>
> ```csharp
> appBuilder.Services.AddOpenTelemetry()
> .WithTracing(builder => builder.AddOtlpExporter("tracing", configure: null))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe provide a non-null configure Action for one of the signals. This might give the wrong impression to people who are new that this the right way to do it:

  1. Provide a null configure Action.
  2. Then,use appBuilder.Services.Configure

Copy link
Member Author

Choose a reason for hiding this comment

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

I just switched it to be specific to binding the options via IConfiguration. If you supply the delegate inline to do code-based configuration you really don't need to use the name at all so I thought this would be better LMK.

@utpilla utpilla merged commit e383b5d into open-telemetry:main Nov 22, 2023
78 checks passed
@CodeBlanch CodeBlanch deleted the otlp-logs-options-di branch November 22, 2023 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logger 'OtlpLogExporter' ignores standard settings if not coming from actual Environment Variables
4 participants