-
Notifications
You must be signed in to change notification settings - Fork 769
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] Support dependency injection in logging build-up #3504
[Logs] Support dependency injection in logging build-up #3504
Conversation
…rceLogEmitter extension.
Codecov Report
@@ Coverage Diff @@
## main #3504 +/- ##
==========================================
+ Coverage 87.23% 87.35% +0.11%
==========================================
Files 275 278 +3
Lines 9959 10083 +124
==========================================
+ Hits 8688 8808 +120
- Misses 1271 1275 +4
|
@@ -22,7 +22,7 @@ | |||
|
|||
var resourceBuilder = ResourceBuilder.CreateDefault().AddService("Examples.LoggingExtensions"); | |||
|
|||
var openTelemetryLoggerProvider = new OpenTelemetryLoggerProvider(options => | |||
var openTelemetryLoggerProvider = OpenTelemetryLoggerProvider.Create(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.
Would a static method on Sdk
make sense? Like Sdk.CreateMeterProviderBuilder
and Sdk.CreateTracerProviderBuilder
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.
Good idea I like it! At the moment the mechanics are slightly different in that there is no Build
method for OpenTelemetryLoggerProvider
. I could introduce a small class LoggerProviderBuilder
so it matches more closely what we have to metrics + traces. Thoughts on that?
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.
If folks aren't thrilled by that idea, then I think Sdk.CreateLoggerProvider
(i.e., w/o Builder
) wouldn't be terrible.
But, consistency would have a nice feel to it, so my vote is a simple class enabling Sdk.CreateLoggerProviderBuilder(...).Build()
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 OK this now has more of a builder pattern. Updated the description. LMK what you think!
Lot of good stuff here! Reviewing these DI/options PRs are always tough for me - always feel like I have to page a bunch of stuff in my head to keep the Rube Goldberg machine straight 😆. Your write up of scenarios is hugely helpful. Also it was helpful for me to take a look at what's been done for other providers to enable these scenarios. I reviewed the ConsoleLoggerExtensions to better grok what you've done here. Regarding your last - maybe most important scenario - I like it! Makes the code for wiring up the EventSource extension clean. |
var openTelemetryLoggerProvider = Sdk.CreateLoggerProviderBuilder( | ||
options => options.IncludeFormattedMessage = true) | ||
.ConfigureResource(builder => builder.AddService("Examples.LoggingExtensions")) | ||
.AddConsoleExporter() | ||
.Build(); |
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.
Feels a little off to have some things configured via the options like IncludeFormattedMessage
and other things configured via methods off the builder.
For metrics we have some similar options like max streams that we set using builder methods:
Sdk.CreateMeterProviderBuilder()
.SetMaxMetricStreams(100);
This PR is growing, so maybe a follow up? Though a follow up would mean the potential for some breaking changes.
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.
Ya this was something I went back and forth on. The bools suck because you can't chain them which kind of breaks the builder pattern. That's why I added the delegate. We could add "Set" methods to preserve the chaining but some downsides to that: More public API and maybe confusing to have properties with get/set and also "Set" methods?
@utpilla @cijothomas @reyang Thoughts?
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.
I guess it all started from SetSampler
.
I personally would find it slightly more user-friendly with builder.SetSomething
if the number of things to be set is small enough, and the things to be set are fairly complex (e.g. very hard to be configuration-based). I would be less happy if there are too many simple options that I have to chain the builder calls:
// it is just more difficult if I want to implement a configuration file
Sdk.CreateMeterProviderBuilder()
.SetMaxMetricStreams(100),
.SetX(true),
.SetY("abc"),
.SetZ(false);
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.
Btw, now that we started talking about configuration, do you think ConfigureResource
should be SetResource
? 😆
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.
@reyang You are going to love this, we actually have SetResourceBuilder
& ConfigureResource
both today 🤣 ConfigureResource was a recent addition.
Subtle differences between the two. Set
clobbers anything that was configured. Configure
is additive. For users configuring their host, I would expect them to call Set
(but they could also Configure
). For libraries/extension authors, they should call Configure
.
We should probably obsolete the Set
version because in Configure
you can call Clear
on the builder if you really wanted to reset it. (Pretty sure, didn't double-check a Clear
exists.)
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.
I would vote for eventually getting rid of ConfigureXyz
(or at least use it carefully) because "configure" is a very general term that can have several different meanings (e.g. it could be additive and commutative; it could be additive but not commutative; it could be idempotent...).
If we're trying to be pedantic, "Append" is probably good for Processor and Exporter (coz it is not commutative due to the sequential processing behavior), "Set" is probably good for Sampler, "Configure" is probably good for anything that takes a complex "XyzOptions" object.
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.
I removed the delegate and replaced it with 3 new "Set" methods. Don't disagree with any of this, it just seems like a bigger conversation we need to have beyond this PR.
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.
+1, I would suggest that we scope this out for now. Maybe revisit it after this PR.
@@ -51,9 +55,41 @@ static Sdk() | |||
/// <param name="textMapPropagator">TextMapPropagator to be set as default.</param> | |||
public static void SetDefaultTextMapPropagator(TextMapPropagator textMapPropagator) | |||
{ | |||
Guard.ThrowIfNull(textMapPropagator); |
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.
I suspect we have other places this question applies, but for public APIs should we notate possible exceptions in the comments?
/// <exception cref="NullReferenceException">
/// Thrown when the <c>textMapPropagator</c> is null.
/// </exception>
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.
I think this would be nice to have, but we would need some tooling to identify.
- would need to discover them all
- would need to keep it up to date when exceptions are added or removed
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.
Totally agree with this. We should have these comments but without tooling support I worry it will often be wrong, missing, or outright lies 😄
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.
Are you saying you're trying to replace good jobs with machines? This PR got political all of the sudden 😆
Jokes aside, yes I completely agree and it's a discussion that's beyond the scope of this PR.
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.
Are you saying you're trying to replace good jobs with machines? This PR got political all of the sudden 😆
Machines probably don't need such <exception>
comment, I assume they will just disassemble the assembly to figure out what exception can be thrown from the callee. 🤣
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.
I think these are great improvements. I think this conversation is a good one for us to follow up on separately #3504 (comment). I agree that settling on using Set*
vs. Configure*
vs. a delegate should be a broader conversation than the goals of this PR.
Merged. There are still some rough edges but now that we're in an alpha/beta stage getting ready for net7 I think it is better to get it in and continue to improve it. |
Changes
Enabled dependency injection scenarios in logging provider build-up similar to what we have for Tracing & Metrics except no dependency on the hosting project is required.
Public API
Scenarios enabled
These are all things that were not possible before.
Register a processor registered through services
Register a processor through services which is automatically configured
Create extension methods which register services and configure provider (for library authors)
Configure the provider after it has been created and the service provider is available
Detached configuration method
If users or library authors wanted to expose some logging registration off of
IServiceCollection
instead ofILoggingBuilder
that is possible like this:Build things that need access to the provider
This is the main reason I worked on this. See: #3454 (comment). OpenTelemetryEventSourceLogEmitter needs to access the provider in order to create a
LogEmitter
. There was previously no way to get access to it when using theILoggingBuilder
API. Now it is possible to build services/extensions like what I am adding on this PR withAddEventSourceLogEmitter
to smooth out the experience:This is done using the
ConfigureProvider
scenario above.It is also possible to do this by registering a detached configuration action directly with services...
TODOs
CHANGELOG.md
updated for non-trivial changes