-
Notifications
You must be signed in to change notification settings - Fork 763
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] Add extension for registering OpenTelemetryLoggerProvider with ILoggingBuilder #3489
Merged
cijothomas
merged 6 commits into
open-telemetry:main
from
CodeBlanch:loggerprovider-extension
Jul 28, 2022
Merged
[Logs] Add extension for registering OpenTelemetryLoggerProvider with ILoggingBuilder #3489
cijothomas
merged 6 commits into
open-telemetry:main
from
CodeBlanch:loggerprovider-extension
Jul 28, 2022
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #3489 +/- ##
=======================================
Coverage 86.49% 86.49%
=======================================
Files 272 272
Lines 9931 9939 +8
=======================================
+ Hits 8590 8597 +7
- Misses 1341 1342 +1
|
alanwest
reviewed
Jul 28, 2022
Comment on lines
-40
to
+48
public static ILoggingBuilder AddOpenTelemetry(this ILoggingBuilder builder, Action<OpenTelemetryLoggerOptions>? configure = null) | ||
public static ILoggingBuilder AddOpenTelemetry(this ILoggingBuilder builder, Action<OpenTelemetryLoggerOptions>? configure) |
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.
🥳 🎉
alanwest
approved these changes
Jul 28, 2022
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.
LGTM
cijothomas
approved these changes
Jul 28, 2022
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Following up on #3454 (comment)
Changes
OpenTelemetryLoggerProvider
with anILoggingBuilder
. You don't really need this extension to do that, but I think it helps with discoverability and our documentation/examples to have it.Details
Here is the use case I am thinking about.
The Serilog startup pattern configures the log pipeline before the host builder. That sort of prescribes that users will need to manually build their own
OpenTelemetryLoggerProvider
. What this new extension does is just smooth that out.Public API
The reason that one method is being removed is I first tried this...
...which gave me the warning:
Symbol 'AddOpenTelemetry' violates the backcompat requirement: 'Public API with optional parameter(s) should have the most parameters amongst its public overloads'. See 'https://github.com/dotnet/roslyn/blob/master/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md' for details.
The layout of the new methods fixes that. Checked with @tarekgh. New design should be mostly safe. If users were calling these methods with reflection and specifically checking the metadata for "optional" parameter, they would break. But I feel like that is a super edge case. Anyone just compiling should be fine.
TODOs
CHANGELOG.md
updated for non-trivial changes