Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove AnonymousType ILLinkTrim entry for EventSource #37083

Merged
merged 1 commit into from
May 29, 2020

Conversation

eerhardt
Copy link
Member

Instead of using an anonymous type to do the logging, just pass the string directly.

Contributes to #35199

It seems reasonable to allow a string to be used as a top-level object in an event, and change StringTypeInfo to use "message" as the name of the field. Thoughts?

I tried a different approach to fixing this. However, that approach ran into dotnet/linker#1218, which prevented it from working. If you prefer to wait for the linker to fix this and use that approach, let me know.

Instead of using an anonymous type to do the logging, just pass the string directly.

Contributes to dotnet#35199
@brianrob
Copy link
Member

I'm not actually sure this will work for tracelogging style events. Were you able to confirm that ETW events produced via this path are consumable?

@brianrob brianrob requested a review from sywhang May 27, 2020 19:50
@eerhardt
Copy link
Member Author

Were you able to confirm that ETW events produced via this path are consumable?

I'm not sure how to test that. Can you tell me? I did run the System.Diagnostics.Tracing tests, which found that I needed to change StringTypeInfo.

As far as I can tell, this path is only used in error cases in ReportOutOfBandMessage.

@brianrob
Copy link
Member

System.Diagnostics.Tracing tests are probably sufficient if you hit an error. There are tests for error paths, and I believe they iterate through the different ways to log events. The one thorny issue is that if you don't run the tests as admin then they don't run the ETW tests - that's the one that I'm most concerned about here.

As you point out, this is used for error paths, so what you'll want to do is trigger an error and then make sure that the error event and any subsequent events can be properly parsed. Here's a test that you can use: https://github.com/brianrob/tests/tree/master/managed/eventsource_error.

Before you run the test, make sure that you're capturing a trace using the following command:

PerfView.exe collect /Providers=*BrokenEventSource,*FunctionalEventSource

Once the app completes (should be quite fast), stop collection and open the Events view. If you filter to just eventsource events and select the two events that we care about, this is what it should look like:

image

The key is that both the error message and the arg="Hello World!" show up properly. This confirms that parsing after the error continues to work properly all the way through the ETW pipeline.

Let me know if you run into any issues with this and I can help.

@eerhardt
Copy link
Member Author

That test worked with my change

image

Note that I changed "Hello World" to be typeof(object).Assembly.Location to ensure that I was using the System.Private.CoreLib.dll from my artifacts folder.

From what I can tell, the reason this works is because if (SelfDescribingEvents) is true for these error events.

Copy link
Member

@brianrob brianrob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eerhardt eerhardt merged commit 45b0d0b into dotnet:master May 29, 2020
@eerhardt eerhardt deleted the RemoveEventSourceILLinkTrim branch May 29, 2020 21:09
eerhardt added a commit to eerhardt/runtime that referenced this pull request Jun 1, 2020
dotnet#36715 and dotnet#37083 modified the CoreCLR version of CoreLib's linker descriptor file, but left them in the Mono version.

dotnet#37255 will address sharing the duplicated parts of these files.
jkotas pushed a commit that referenced this pull request Jun 1, 2020
#36715 and #37083 modified the CoreCLR version of CoreLib's linker descriptor file, but left them in the Mono version.

#37255 will address sharing the duplicated parts of these files.
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants