EventSourceGenerator for Guid+Name .ctor and Metadata

[benaadams]

Haven't done the manifest generation; which is the largest, because it's a bit wild...

Contributes to #44598

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Haven't done the manifest generation; which is the largest, because it's a bit wild...

Contributes to #44598

Author:	benaadams
Assignees:	-
Labels:	area-System.Diagnostics.Tracing
Milestone:	-

[tommcdon]

@josalem @sywhang

[danmoseley]

I think this will complete #43390 ? Anyhow, that's the user story that includes this. cc @tommcdon

edit: No, as you say this is just part.

[benaadams]

I think this will complete #43390 ?

Also needs Manifest generation and all the EventSource apis are private so can't be used publicly and would need api review?

[stephentoub]

Also needs Manifest generation and all the EventSource apis are private so can't be used publicly and would need api review?

And on top of that, as nice a start as this is (thanks, @benaadams), it needs to apply to much more than just the EventSources in corelib to consider that issue addressed. That includes all the other EventSources in netcoreapp and aspnetcoreapp, and then ideally available for 3rd-party use as well.

[benaadams]

Need to rebase after #45738 is merged since I'm changing the .sln file in this PR

[benaadams]

Rebased

[benaadams]

runtime (CoreCLR Pri0 Runtime Tests Run Linux x64 checked) error is #39669

Unhandled exception. System.Exception: 
   Profiler tests are expected to contain the text 'PROFILER TEST PASSES' in the console output of the profilee app to indicate a passing test. 
   Usually it is printed from the Shutdown() method of the profiler implementation. This text was not found in the output above.
   at Profiler.Tests.ProfilerTestRunner.FailFastWithMessage(String error) in /__w/1/s/src/tests/profiler/common/ProfilerTestRunner.cs:line 146
   at Profiler.Tests.ProfilerTestRunner.Run(String profileePath, String testName, Guid profilerClsid, String profileeArguments, ProfileeOptions profileeOptions)
      in /__w/1/s/src/tests/profiler/common/ProfilerTestRunner.cs:line 94
   at EventPipeTests.EventPipe.Main(String[] args) in /__w/1/s/src/tests/profiler/eventpipe/eventpipe.cs:line 31
   
/home/helixbot/work/BC020A6B/w/B1F50981/e/profiler/eventpipe/eventpipe/eventpipe.sh:
 line 379: 10183 Aborted                 (core dumped) $LAUNCHER $ExePath "${CLRTestExecutionArguments[@]}"

Return code:      1
Raw output file:      /home/helixbot/work/BC020A6B/w/B1F50981/e/profiler/eventpipe/Reports/profiler.eventpipe/eventpipe/eventpipe.output.txt
Raw output:
BEGIN EXECUTION
/home/helixbot/work/BC020A6B/p/corerun eventpipe.dll ''
Profiler path: /home/helixbot/work/BC020A6B/w/B1F50981/e/profiler/eventpipe/eventpipe/libProfiler.so
Profiler.dll!DllGetClassObject
Profiler.dll!Profiler::Initialize
Gathering state for process 10207 corerun
Writing minidump with heap to file /home/helixbot/dotnetbuild/dumps/coredump.10207.dmp
Written 84791296 bytes (20701 pages) to core file
Dump successfully written

Test failed: Profiler tests are expected to contain the text 'PROFILER TEST PASSES' in the console output of the profilee app to indicate a passing test. Usually it is printed from the Shutdown() method of the profiler implementation. This text was not found in the output above.
Gathering state for process 10183 corerun
Writing minidump with heap to file /home/helixbot/dotnetbuild/dumps/coredump.10183.dmp
Written 98660352 bytes (24087 pages) to core file
Dump successfully written
Expected: 100
Actual: 134
END EXECUTION - FAILED
Test Harness Exitcode is : 1
To run the test:
> set CORE_ROOT=/home/helixbot/work/BC020A6B/p
> /home/helixbot/work/BC020A6B/w/B1F50981/e/profiler/eventpipe/eventpipe/eventpipe.sh
Expected: True
Actual:   False

[josalem]

CC @davmason for the profiler test failure, though I suspect the dump from that run is long gone.

[josalem]

Forgot to mention in my previous comment: I'm currently reviewing this PR. I'm no expert on the Roslyn Code Generator APIs, though.

[josalem]

Overall, this looks good, but we need to make sure it doesn't add any public types to SPC.

[josalem]

Another thing to note: I'm not sure we have any source generators in dotnet/runtime right now. This would be the first. @danmosemsft @ericstj has there been any motion on your comment chain #45105 (comment)?

[benaadams]

This generator will only be used (for now) with EventSources in SPC, correct?

Yes it would need some of the EventSources apis (+api review) to be made public to be used outside

[tommcdon]

cc @noahfalk

[stephentoub]

Cool. Thanks, @benaadams.

[stephentoub]

Clever, eliminates the parsing at execution time.

[benaadams]

Rebased to restart CI

[stephentoub]

Haven't done the manifest generation

I wonder if, until we can tackle that, it'd be worth adding another internal virtual property that we override on the sources in corelib and hardcode to return the appropriate metadata string/bytes? It would mean we'd need to manually update it any time we wanted to augment or otherwise change the events on these sources, until a source generator would take over.

[benaadams]

I wonder if, until we can tackle that, it'd be worth adding another internal virtual property that we override on the sources in corelib and hardcode to return the appropriate metadata string/bytes?

Can do in follow up?

[stephentoub]

Of course, assuming we even want to do that.

[josalem]

Hey @benaadams, thanks again for this PR! Sorry for the long review time. We're actively thinking about how to handle this very situation end-to-end and this work gives us a great starting point. The end state of this work will probably result in new public APIs on EventSource which is what is causing the slow down.

The conversation on your other PR seems to show consensus on the location of the Source Generator code. I'll try to get another review out soon.

As for the manifest generation work (the bit that will probably require a new public API), there is some prior art from the .NET Native days:

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

Lines 646 to 663 in dbcea6b

	internal virtual void GetMetadata(out Guid eventSourceGuid, out string eventSourceName, out EventMetadata[] eventData, out byte[] manifestBytes)
	{
	//
	// In ProjectN subclasses need to override this method, and return the data from their EventSourceAttribute and EventAttribute annotations.
	// On other architectures it is a no-op.
	//
	// eventDescriptors needs to contain one EventDescriptor for each event; the event's ID should be the same as its index in this array.
	// manifestBytes is a UTF-8 encoding of the ETW manifest for the type.
	//
	// This will be implemented by an IL rewriter, so we can't make this method abstract or the initial build of the subclass would fail.
	//
	eventSourceGuid = Guid.Empty;
	eventSourceName = null;
	eventData = null;
	manifestBytes = null;
	return;
	}

[josalem]

Thanks @benaadams! I'll look at merging later this afternoon tomorrow morning to give @jkotas, @ericstj, or other reviewers time to chime in since this is modifying SPCL.

[josalem]

@jaredpar is there a canonical way to do this with source generators? I'm not opposed to this method, though.

[benaadams]

private partial ArrayPoolEventSource(); would have worked if it was allowed (though isn't, only works for regular methods, not constructors)

The main aim is to fail compilation if the generator didn't run; rather than to succeed but not use the generated paths.

[josalem]

Pardon the delay, I was unexpectedly out of office yesterday. I'll be merging this morning 😄