Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add otlp log extension methods for LoggerProviderBuilder #5103
Changes from 38 commits
f206708
aa42126
b0c97f8
e11d4bb
36e8560
6723a04
3653fd3
64f8df9
b15421a
4255964
0b853e7
3e2aff0
ac70c78
06893fe
7f28ee5
5fe64d3
c828fcc
6c4d67b
7276894
7a7e962
df0adcb
6cf59f0
f08622d
35406ae
1f99014
ac0494b
a350e30
fa4d229
04a036c
d09f1b0
fa2857a
604ccf6
c19868d
e208fbc
c4e795f
2f51df2
e17f4e7
66ea5e3
8f15ca0
11d7f4c
df362e4
a332701
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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 forSdkLimitOptions
andExperimentalOptions
.Shouldn't we be using
IOptionsFactory<TOptions>.Create
method to create these options?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
SdkLimitOptions
andExperimentalOptions
will be registered into theIOptionsFactory<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
.There was a problem hiding this comment.
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
andExperimentalOptions
, shouldn't we be using either:IOptionsFactory<TOptions>.Create
orIOptionsMonitor<TOptions>.Get
to get these options. Shouldn't we be utilizing theOptionsFactory
that we are now registering in this PR for these two options?There was a problem hiding this comment.
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
andExperimentalOptions
because they areinternal
but I went ahead and refactored it so it is nice and consistent now.