Make AddFilter chainable, add some doc comments, add UseConfiguration… #597

BrennanConroy · 2017-04-12T15:32:42Z

… to LoggerFactory

glennc · 2017-04-12T16:08:59Z

samples/SampleApp/Program.cs

-                { "SampleApp.Program", LogLevel.Debug }
-            });
+            var factory = new LoggerFactory()
+                .UseConfiguration(loggingConfiguration.GetSection("Logging"))


Not sure of this name. It kind of implies that you are configuring the factory, like adding providers, instead of just filtering.

BrennanConroy · 2017-04-14T16:15:04Z

@Eilon Some doc comments

Eilon · 2017-04-19T17:28:34Z

src/Microsoft.Extensions.Logging/LoggerFactory.cs

+        /// <summary>
+        /// Replaces the <see cref="IConfiguration"/> used for filtering.
+        /// </summary>
+        /// <param name="configuration">The replacing configuration.</param>


The new configuration to use. ?

Eilon · 2017-04-19T17:30:40Z

src/Microsoft.Extensions.Logging/LoggerFactory.cs

-        public void AddFilter(string providerName, string categoryName, Func<LogLevel, bool> filter)
+        /// <summary>
+        /// Adds a filter that applies to <paramref name="providerName"/> and <paramref name="categoryName"/> with the given
+        /// <paramref name="filter"/>, returning true means allow log through, false means reject log.


Remove the , returning ... section because it's just describing the parameter. Put that text in the description of how ti implement the filter parameter.

Eilon · 2017-04-19T17:33:59Z

src/Microsoft.Extensions.Logging/LoggerFactory.cs

+        }
+
+        /// <summary>
+        /// Replaces the <see cref="IConfiguration"/> used for filtering.


I'm a bit late on the design here, but why does this replace the configuration used for filtering? What if I have 2 sets of config that I want to use from 2 different sections? This seems to me like it should all be additive, always.

I think usage will mostly be factory.UseConfiguration(config.GetSection("Logging"))
with the config having:

{ "OtherStuff": { }, "Logging": { "LogLevel": { "System": "Trace" } } }

When would you want to use multiple sections?

Why would we specifically disallow it?

I'm ok punting on this now; let's log a bug to track this discussion.

Eilon · 2017-04-19T17:39:23Z

test/Microsoft.Extensions.Logging.Test/LoggerFilterTest.cs

            var writes = provider.Sink.Writes;
            Assert.Equal(1, writes.Count);
        }



A bunch of code changed in this PR - should there not be more tests for this? E.g. the new extension methods, the new UseConfiguration() method, etc.

BrennanConroy · 2017-04-24T16:14:06Z

🆙 📅

BrennanConroy · 2017-04-24T18:05:53Z

Need to do aspnet/MetaPackages#45 and Remove passing logging section by default in hosting after this change.

Eilon · 2017-04-25T04:51:23Z

src/Microsoft.Extensions.Logging/LoggerFactory.cs

-        public void AddFilter(string providerName, Func<string, LogLevel, bool> filter)
+        /// <summary>
+        /// Adds a filter that applies to <paramref name="providerName"/> with the given
+        /// <paramref name="filter"/>, returning true means allow log through, false means reject log.


These are two separate sentences. Also, the second part of the sentence (line 2) really just applies to the filter param, so should put that comment there. I think this was corrected in the previous comments, so I recommend using that same pattern for all of them.

Eilon · 2017-04-25T04:55:21Z

src/Microsoft.Extensions.Logging/LoggerFactoryExtensions.cs

+
+        /// <summary>
+        /// Adds a filter that applies to <paramref name="providerName"/> with the given
+        /// <paramref name="filter"/>, returning true means allow log through, false means reject log.


See earlier comments, apply to the extension methods as well.

Eilon · 2017-04-25T04:57:37Z

src/Microsoft.Extensions.Logging/LoggerFactoryExtensions.cs

+
+namespace Microsoft.Extensions.Logging
+{
+    public static class LoggerFactoryExtensions


Add a class-level doc comment as well. Could be as simple as something like: https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc/MvcServiceCollectionExtensions.cs#L15-L17

Eilon · 2017-04-25T05:02:14Z

src/Microsoft.Extensions.Logging/LoggerFactoryExtensions.cs

+        /// <param name="categoryName">The name of the logger category.</param>
+        /// <param name="minLevel">The minimum <see cref="LogLevel"/> that logs from
+        /// <paramref name="providerName"/> and <paramref name="categoryName"/> are allowed.</param>
+        public static LoggerFactory AddFilter(this LoggerFactory factory, string providerName, string categoryName, LogLevel minLevel)


General question: Why are some of these extension methods, but some are methods on LoggerFactory itself?

Performance mainly

Wuh?? Performance of what?

Talked in person, the perf is about optimizing the instance methods because they can do smarter stuff w/ data structures. But then there's no reason to have extension methods, they might as well all be instance.

Eilon · 2017-04-25T05:03:30Z

test/Microsoft.Extensions.Logging.Test/LoggerFilterTest.cs

+            writes = loggerProvider.Sink.Writes;
+            Assert.Equal(1, writes.Count);
+        }
+


Umm... missing something here??? 😄

Hahaha! Maybe thats why it passes ;)

Eilon · 2017-04-25T05:09:56Z

src/Microsoft.Extensions.Logging/LoggerFactory.cs

+        /// <param name="filter">The filter that applies to logs for <paramref name="providerName"/> and <paramref name="categoryName"/>,
+        /// returning true means allow log through, false means reject log.</param>
+        /// <returns>The <see cref="LoggerFactory"/> so that additional calls can be chained.</returns>
+        public LoggerFactory AddFilter(string providerName, string categoryName, Func<LogLevel, bool> filter)


Maybe add a null check for filter?

Eilon · 2017-04-25T05:13:53Z

test/Microsoft.Extensions.Logging.Test/LoggerFilterTest.cs

            // Assert
            writes = loggerProvider.Sink.Writes;
            Assert.Equal(1, writes.Count);
        }


Do all the filter methods (whether instance methods or extension methods) have at least one or two tests each? (For whatever interesting combinations of params they have.)

Yes, made sure they all have at least 1 test. I wanted to do a programmatic way of testing all the filters with each other etc. but it was going to be complex and crazy so...

… to LoggerFactory

dnfclas added the cla-already-signed label Apr 12, 2017

BrennanConroy requested review from glennc and kichalla April 12, 2017 16:03

glennc reviewed Apr 12, 2017

View reviewed changes

BrennanConroy requested a review from pakrym April 13, 2017 16:17

Eilon suggested changes Apr 19, 2017

View reviewed changes

BrennanConroy force-pushed the brecon/fixup branch from 15aacad to 1f8e600 Compare April 24, 2017 16:13

Eilon suggested changes Apr 25, 2017

View reviewed changes

BrennanConroy force-pushed the brecon/fixup branch from 1f8e600 to 0fb4b65 Compare April 25, 2017 16:25

Eilon approved these changes Apr 25, 2017

View reviewed changes

BrennanConroy added 4 commits April 25, 2017 14:39

Make AddFilter chainable, add some doc comments, add UseConfiguration…

4cc77e9

… to LoggerFactory

feedback

87fafde

fb

1bbc30d

fb

8c0cde4

BrennanConroy force-pushed the brecon/fixup branch from 5359618 to 8c0cde4 Compare April 25, 2017 21:53

BrennanConroy merged commit eecb8e2 into dev Apr 25, 2017

BrennanConroy deleted the brecon/fixup branch April 25, 2017 22:03

Make AddFilter chainable, add some doc comments, add UseConfiguration… #597

Make AddFilter chainable, add some doc comments, add UseConfiguration… #597

Uh oh!

Conversation

BrennanConroy commented Apr 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrennanConroy commented Apr 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrennanConroy commented Apr 24, 2017

Uh oh!

BrennanConroy commented Apr 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants