Changes to Logging API surface

[muratg]

• Have all levels (Critical, Debug, Error, Warning, Information, Verbose) have the same overloads as extension methods:
  • Log....(this ILogger logger, EventId eventId, Exception exception, string message, params object[] args);
  • Log....(this ILogger logger, EventId eventId, string message, params object[] args);
  • Log....(this ILogger logger, string message, params object[] args);
• EventId has implicit conversion from/to Int and if you want to use string, you do the conversion yourself
• Our frameworks will be passing in both int and string based EventIds.
• Event IDs (string versions) in our frameworks should be PascalCased and should typically be two words. Some examples:
  • "QueryExecuting"
  • "ActionExecuting"
  • "ContentNegotiationFailed"
  • ...
• Change the LoggerMessage.Define to take an EventId instead of int
  • There may be other places where int eventId could be used. Convert them to EventId.
• MessageFormatter should be state to string (and not do formatting at all)
• MessageFormatter state should never be null, null check shouldn’t be done in runtime
• Remove ILogValues and use IReadOnlyList<KeyValuePair<string, object>>
• ILogger change: Instead of void Log(LogLevel logLevel, int eventId, object state, Exception exception, Func<object, Exception, string> formatter) we should use void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception exception, Func<TState, Exception, string> formatter)

[muratg]

cc @Eilon @rynowak @lodejard @davidfowl @divega @bricelam

[muratg]

We will start this work shortly.

[NickCraver]

Might I suggest a [breaking-change] label rather than (or in addition to) [bug] for these? That will help library authors a great deal in filtering what's most important in the future.

[davidfowl]

We do that on the announcements repository not here

[NickCraver]

@davidfowl that may work for the Microsoft side, but consumers aren't going to keep checking the monolithic announcements repo and filter out the noise to see what affects them on every change. Have you actually tried to consume parts of that? What's the harm in a label on the specific issues as well? It's really very little overhead and is beneficial. On top of that, bug is incorrect - most of these items aren't bugs.

To be clear: you guys are doing a great job, but your consumption and generation model of information is very different (and has to be) than those focusing on specific parts of the framework. If we can very trivially make life easier for the latter group: why not? At the very least, issues like this should link to the breaking change announcement if that's the only place they live. It shouldn't be on every user to read every bug here and determine it's a breaking change.

[davidfowl]

Have you actually tried to consume parts of that?

Yes, it's actually very good for breaking API changes.

What's the harm in a label on the specific issues as well?

We need to add it to the other 50 repos. We already settled on a process for tracking breaking changes across this monolithic 1.0 release and that's the Announcements repository. We can revisit after RTM when each of the projects end up on their own release cadences.

You'll need to do it until we RTM. It's how we're tracking breaking change like this and it isn't that hard. We summarize the changes and write up what you need to change in your apps in future milestones. Also, the announcements are grouped into milestones so it'll give you an overview of what changed.

[NickCraver]

@davidfowl I'm aware of the motivations, just be aware it's not a well received method on the outside. There's no discussion allowed on the one place you're "tracking" breaking changes, and that's not a good thing. As for the grouping: grouping in milestones is for schedules and that's totally understandable. But users have code that touches specific points. Library authors are a more extreme case of touching and caring about very specific areas. The announcements repo isn't good for this. Keep in mind you're likely in the meeting or aware of the issues outside of GitHub; many more of us aren't - and they're not as easy to find because you don't know what to grep for.

When a new release happens like RC2 or RTM, I assume you'll want library authors to upgrade quickly so everyone downstream can upgrade as well. I'm simply proposing helping them do that better and faster.

Take from that what you will, but the biggest complaint I (still) hear often about .Net Core is how unapproachable it is. If there's no real willingness to fix that, the price will unfortunately be in the adoption numbers.

[davidfowl]

@NickCraver I hear ya but we're not going to change the process for 1.0, it's approaching at a rapid pace and the current model does work. We can look at improving it post 1.0 but we're not going to change our entire process of tracking breaking changes right now, we don't have the time.

PS: This has nothing to do with .NET Core, this is a logging abstraction that's used by ASP.NET applications and libraries. It's like any other logging abstraction out there and we're making some more breaking changes for RC2.

[NickCraver]

@davidfowl I wouldn't expect a major change - consider this feedback for improving as the schedule allows. This point still stands though:

At the very least, issues like this should link to the breaking change announcement if that's the only place they live.

There's no link here to any breaking change announcement, in either direction. Is that too much to ask? It's not a big process change.

[Eilon]

@NickCraver this change hasn't been committed yet, hence no announcement or discussion yet either. It's still being worked on.

[davidfowl]

That's also why this bug is open 😄 and labeled as "working" not "done".

[NickCraver]

Apologies, since the latest issue in announcements and the other 3 from logging I looked at didn't have links back to any issues in this repo I didn't think any did - I see most others actually have a proper link. I still think there's benefit in alerting people earlier (so we can schedule work as well), but also see a process change pre-1.0 isn't likely.

[NickCraver]

Oh and FWIW, @davidfowl :

PS: This has nothing to do with .NET Core, this is a logging abstractions that's used by ASP.NET applications.

I wish it was in coreclr, or that any consistent interface was for handling logging in general. I'm porting over our exception handling libraries (which will implement the abstractions here) and wish it was the same for non-aspnet applications as well. (The current tagline "Common logging abstractions for DNX and ASP.NET 5." isn't clear on intent there) As it stands I can't find a way to see all unhandled exceptions, but that's an issue by itself - will file in a bit.

[BrennanConroy]

4528bdb