Adding a simple switch to exclude unnecessary activations #35
Conversation
srogovtsev
changed the title
srogovtsev
changed the title
srogovtsev
force-pushed
the
feature/limiting-preparing-handlers
branch
from
0aafd1b to
db03a39
Compare
| @@ -19,11 +19,15 @@ public static class SerilogContainerBuilderExtensions | |||
| /// <param name="autowireProperties">If true, properties on reflection-based components of type <see cref="ILogger"/> will | |||
| /// be injected.</param> | |||
| /// <param name="dispose"></param> | |||
| /// <param name="onlyKnownConsumers"> | |||
| /// If true, only the registrations that can be verified to use <see cref="ILogger"/> will be modified to access the logger, | |||
| /// thus avoiding unnecessary logger calls. | |||
There was a problem hiding this comment.
Maybe "unnecessary reflection"?
There was a problem hiding this comment.
It's actually not about reflection.
var log = args.Context.Resolve<ILogger>().ForContext(registration.Activator.LimitType);Probably "resolution" would be most appropriate.
(BTW, this line has quite a potential for improving, because you don't have to resolve ILogger from container, you have all the information you need right here - and you'll skip redundant NamedParameter check as well)
| @@ -26,11 +27,12 @@ public ContextualLoggingModule() | |||
| _skipRegistration = true; | |||
| } | |||
|
|
|||
| internal ContextualLoggingModule(ILogger logger = null, bool autowireProperties = false, bool dispose = false) | |||
| internal ContextualLoggingModule(ILogger logger = null, bool autowireProperties = false, bool dispose = false, bool onlyKnownConsumers = false) | |||
There was a problem hiding this comment.
Perhaps the name should be reflectionActivatorsOnly?
There was a problem hiding this comment.
I feel this would be a little too specific and breaking encapsulation. But you want to invert it anyway, so this is probably a moot point.
|
Seems like a nice "contained" fix; I'd be inclined to consider a breaking change, and reverse this so it's an opt-out, e.g. the param would be |
I'd go for |
|
Thanks for the reply! Been trying to come up with another suggestion all week :-) - I think "unsure" could be a confusing term; would |
…xplicitly asked to * decided against providing to ProvidedInstanceActivator
|
Here you have it. Sorry for the delay, we seem to be working from quite different time zones. Just for the reference, I've decided to explicitly exclude |
|
Fantastic PR - many thanks. I'll queue up a release PR to merge once the dev package has been in circulation for a litte while 👍 |
An option 3 from #34: