[Group 4] Enable nullable annotations for `Microsoft.Extensions.Logging.EventSource` by maxkoshevoi · Pull Request #66802 · dotnet/runtime

maxkoshevoi · 2022-03-17T23:46:55Z

Related to #43605

Questions:

Arguments can be null here

runtime/src/libraries/Microsoft.Extensions.Logging.EventSource/src/LoggingEventSource.cs

Line 303 in 731d936

if (!command.Arguments.TryGetValue("FilterSpecs", out string filterSpec))
I'm unable to build this repository for the past couple of weeks, installing .Net 7 Preview 3 from main doesn't help. Is there something I'm missing?

CS0433	The type 'Unsafe' exists in both 'System.Runtime.CompilerServices.Unsafe, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' and 'System.Runtime, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'	Microsoft.Extensions.Logging.EventSource (net7.0)

ghost · 2022-03-18T15:05:29Z

Tagging subscribers to this area: @dotnet/area-extensions-logging
See info in area-owners.md if you want to be subscribed.

Issue Details

Related to #43605

Questions:

Arguments can be null here

runtime/src/libraries/Microsoft.Extensions.Logging.EventSource/src/LoggingEventSource.cs

Line 303 in 731d936

if (!command.Arguments.TryGetValue("FilterSpecs", out string filterSpec))
I'm unable to build this repository for the pas couple of weeks, installing .Net 7 Preview 3 from main doesn't help. Is there something I'm missing?

CS0433	The type 'Unsafe' exists in both 'System.Runtime.CompilerServices.Unsafe, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' and 'System.Runtime, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'	Microsoft.Extensions.Logging.EventSource (net7.0)

Author:	maxkoshevoi
Assignees:	-
Labels:	`area-Extensions-Logging`, `community-contribution`
Milestone:	-

eerhardt · 2022-03-18T15:06:35Z

I'm unable to build this repository for the pas couple of weeks, installing .Net 7 Preview 3 from main doesn't help. Is there something I'm missing?

I would try cleaning my repo: git clean -xdf and then rebuild from the root .\build.cmd libs+clr -rc Release. If you still have errors, let me know.

eerhardt · 2022-03-18T15:10:35Z

...ies/Microsoft.Extensions.Logging.EventSource/ref/Microsoft.Extensions.Logging.EventSource.cs

    {
        public EventSourceLoggerProvider(Microsoft.Extensions.Logging.EventSource.LoggingEventSource eventSource) { }
-        public Microsoft.Extensions.Logging.ILogger CreateLogger(string categoryName) { throw null; }
+        public Microsoft.Extensions.Logging.ILogger CreateLogger(string? categoryName) { throw null; }


ILoggerProvider has categoryName as non-nullable. So this should be string categoryName.

We don't want to allow this to be null.

Should EventSourceLogger.CategoryName be nullable then? Maybe it doesn't really matter sines it's an internal API, and nothing breaks when it's nullable, but still.

Should EventSourceLogger.CategoryName be nullable then?

I would say no. If nothing ever creates it with a null name, then I wouldn't make it nullable.

src/libraries/Microsoft.Extensions.Logging.EventSource/src/LoggingEventSource.cs

src/libraries/Microsoft.Extensions.Logging.EventSource/src/ExceptionInfo.cs

src/libraries/Microsoft.Extensions.Logging.EventSource/src/EventSourceLoggerProvider.cs

src/libraries/Microsoft.Extensions.Logging.EventSource/src/EventSourceLogger.cs

eerhardt · 2022-03-21T17:14:02Z

/azp run

azure-pipelines · 2022-03-21T17:14:08Z

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

eerhardt · 2022-03-21T17:14:21Z

/azp run runtime

azure-pipelines · 2022-03-21T17:14:51Z

Azure Pipelines successfully started running 1 pipeline(s).

eerhardt

LGTM. Thanks for the contribution here, @maxkoshevoi!

eerhardt · 2022-03-23T21:01:25Z

src/libraries/Microsoft.Extensions.Logging.EventSource/src/LoggingEventSource.cs

+            if (command.Command is EventCommand.Update or EventCommand.Enable)
            {
-                if (!command.Arguments.TryGetValue("FilterSpecs", out string filterSpec))
+                if (!command.Arguments!.TryGetValue("FilterSpecs", out string? filterSpec))


This is the only place that concerns me. But if it hasn't been a problem, I guess we can just suppress the null check here. Looking around I see similar code elsewhere:

runtime/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticSourceEventSource.cs

Lines 393 to 397 in c5d40c9

if ((command.Command == EventCommand.Update || command.Command == EventCommand.Enable) &&

IsEnabled(EventLevel.Informational, Keywords.Events))

{

string? filterAndPayloadSpecs = null;

command.Arguments!.TryGetValue("FilterAndPayloadSpecs", out filterAndPayloadSpecs);

runtime/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/CounterGroup.cs

Lines 53 to 59 in c5d40c9

private void OnEventSourceCommand(object? sender, EventCommandEventArgs e)

{

if (e.Command == EventCommand.Enable || e.Command == EventCommand.Update)

{

Debug.Assert(e.Arguments != null);

if (e.Arguments.TryGetValue("EventCounterIntervalSec", out string? valueStr) && float.TryParse(valueStr, out float value))

danmoseley · 2022-03-23T22:47:35Z

+1 we appreciate your hard work on all these annotations @maxkoshevoi .

…ng.EventSource` (dotnet#66802) * Annotate src * Annotate ref * Make internal parameters non-nullable if they never receive null

maxkoshevoi added 2 commits March 18, 2022 01:32

Annotate src

f707673

Annotate ref

cc1822a

ghost added the community-contribution Indicates that the PR has been added by a community member label Mar 17, 2022

eerhardt added the area-Extensions-Logging label Mar 18, 2022

eerhardt reviewed Mar 18, 2022

View reviewed changes

src/libraries/Microsoft.Extensions.Logging.EventSource/src/LoggingEventSource.cs Outdated Show resolved Hide resolved

eerhardt reviewed Mar 18, 2022

View reviewed changes

src/libraries/Microsoft.Extensions.Logging.EventSource/src/ExceptionInfo.cs Outdated Show resolved Hide resolved

eerhardt reviewed Mar 18, 2022

View reviewed changes

src/libraries/Microsoft.Extensions.Logging.EventSource/src/EventSourceLoggerProvider.cs Outdated Show resolved Hide resolved

eerhardt reviewed Mar 18, 2022

View reviewed changes

src/libraries/Microsoft.Extensions.Logging.EventSource/src/EventSourceLogger.cs Outdated Show resolved Hide resolved

maxkoshevoi added 4 commits March 18, 2022 23:00

Merge branch 'main' into mk/43605-Logging-EventSource

99e1b47

string categoryName

e92fd25

Reverse "Use pattern matching"

1930662

Other code review

acad275

maxkoshevoi marked this pull request as ready for review March 18, 2022 21:15

Make internal parameters non-nullable if they never receive null

a9b3890

This was referenced Mar 21, 2022

ThreadPoolTests.CooperativeBlockingCanCreateThreadsFaster timed out #64964

Closed

Threadpool test CooperativeBlockingCanCreateThreadsFaster failing on Mac #66852

Closed

eerhardt approved these changes Mar 23, 2022

View reviewed changes

eerhardt merged commit 103fb84 into dotnet:main Mar 23, 2022

maxkoshevoi mentioned this pull request Mar 23, 2022

Fix nullable annotations in EventSourceLogger #67065

Merged

maxkoshevoi deleted the mk/43605-Logging-EventSource branch March 23, 2022 21:30

ghost locked as resolved and limited conversation to collaborators Apr 23, 2022

	if ((command.Command == EventCommand.Update \|\| command.Command == EventCommand.Enable) &&
	IsEnabled(EventLevel.Informational, Keywords.Events))
	{
	string? filterAndPayloadSpecs = null;
	command.Arguments!.TryGetValue("FilterAndPayloadSpecs", out filterAndPayloadSpecs);

	private void OnEventSourceCommand(object? sender, EventCommandEventArgs e)
	{
	if (e.Command == EventCommand.Enable \|\| e.Command == EventCommand.Update)
	{
	Debug.Assert(e.Arguments != null);

	if (e.Arguments.TryGetValue("EventCounterIntervalSec", out string? valueStr) && float.TryParse(valueStr, out float value))

Conversation

maxkoshevoi commented Mar 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Mar 18, 2022

Uh oh!

eerhardt commented Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eerhardt Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

maxkoshevoi Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

eerhardt Mar 21, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerhardt commented Mar 21, 2022

Uh oh!

azure-pipelines bot commented Mar 21, 2022

Uh oh!

eerhardt commented Mar 21, 2022

Uh oh!

azure-pipelines bot commented Mar 21, 2022

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

eerhardt Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Mar 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

maxkoshevoi commented Mar 17, 2022 •

edited

Loading

eerhardt commented Mar 18, 2022 •

edited

Loading