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

Add otlp log extension methods for LoggerProviderBuilder #5103

Merged
merged 42 commits into from
Dec 23, 2023

Conversation

Yun-Ting
Copy link
Contributor

@Yun-Ting Yun-Ting commented Nov 30, 2023

Towards #4433
Based on #4596

Changes

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)

Copy link

codecov bot commented Dec 1, 2023

Codecov Report

Attention: 41 lines in your changes are missing coverage. Please review.

Comparison is base (e3759a1) 83.48% compared to head (a332701) 83.19%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5103      +/-   ##
==========================================
- Coverage   83.48%   83.19%   -0.30%     
==========================================
  Files         297      297              
  Lines       12395    12530     +135     
==========================================
+ Hits        10348    10424      +76     
- Misses       2047     2106      +59     
Flag Coverage Δ
unittests 83.19% <74.05%> (-0.30%) ⬇️

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

Files Coverage Δ
...try/Logs/ILogger/OpenTelemetryLoggingExtensions.cs 97.40% <100.00%> (+2.16%) ⬆️
...lemetryProtocol/OtlpLogExporterHelperExtensions.cs 71.06% <65.54%> (-18.77%) ⬇️

... and 8 files with indirect coverage changes

@Yun-Ting Yun-Ting marked this pull request as ready for review December 12, 2023 00:45
@Yun-Ting Yun-Ting requested a review from a team December 12, 2023 00:45
@CodeBlanch CodeBlanch added pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package logs Logging signal related labels Dec 19, 2023
@CodeBlanch
Copy link
Member

@utpilla @vishweshbankwar I'm not going to approve this because I did some of the work and I don't want to self-approve but just FYI I think it is in a good state for final review/merge.


var config = sp.GetRequiredService<IConfiguration>();
var config = sp!.GetRequiredService<IConfiguration>();

var sdkLimitOptions = new SdkLimitOptions(config);
Copy link
Contributor

Choose a reason for hiding this comment

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

The newly added AddOtlpLogExporter methods are registering OptionsFactory for SdkLimitOptions and ExperimentalOptions.

Shouldn't we be using IOptionsFactory<TOptions>.Create method to create these options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The SdkLimitOptions and ExperimentalOptions will be registered into the IOptionsFactory<T> which takes IConfiguration as one of the parameters:
https://github.com/open-telemetry/opentelemetry-dotnet/blob/e3759a1e0ea6e23b80cad33a206795fda8e240ff/src/Shared/Options/ConfigurationExtensions.cs#L137C1-L145C12
So the options can be retrieved when constructing new option instances with IConfiguration.

Copy link
Contributor

Choose a reason for hiding this comment

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

My question is instead of using the regular ctor for SdkLimitOptions and ExperimentalOptions, shouldn't we be using either: IOptionsFactory<TOptions>.Create or IOptionsMonitor<TOptions>.Get to get these options. Shouldn't we be utilizing the OptionsFactory that we are now registering in this PR for these two options?

Copy link
Member

Choose a reason for hiding this comment

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

It isn't as important to use Options API with SdkLimitOptions and ExperimentalOptions because they are internal but I went ahead and refactored it so it is nice and consistent now.

@utpilla utpilla merged commit 0889e8d into open-telemetry:main Dec 23, 2023
78 checks passed
@Yun-Ting Yun-Ting deleted the yunl/OtlpLoggingExtension branch December 23, 2023 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logs Logging signal related pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants