search

google / UIforETW

User interface for recording and managing ETW traces
https://randomascii.wordpress.com/2015/04/14/uiforetw-windows-performance-made-easier/
Apache License 2.0
1.55k stars 201 forks source link

Added support for TraceLogging providers #100

Closed mwinterb closed 7 years ago

mwinterb commented 7 years ago

Only strictly necessary for pre-Windows 8 operating systems as the xperf from the Windows 10 Anniversary Edition version (August 2016) SDK supports the proper translation "out of the box".

The algorithm to translate from TraceLogging provider name to GUID is based on the C# code posted by Doug Cook to https://blogs.msdn.microsoft.com/dcook/2015/09/08/etw-provider-names-and-guids/

mwinterb commented 7 years ago

This is partly a WIP to solicit feedback. I've tested it with my two TraceLogging providers with extra options and it's functional, but if there are objections to the general path I'd rather find out sooner.

randomascii commented 7 years ago

Roughly speaking this seems good. I may not get a chance to try the code until next week.

If the change is squashed that is better - having it as a single commit is nice - but I doubt that is crucial.

randomascii commented 7 years ago

Much to my surprise it looks like the kernel-callback-exception-swallowing has been fixed. Crazy. I wouldn't have thought Microsoft would be brave enough to do that.

randomascii commented 7 years ago

Just a few tidiness things, if you don't mind. The issues are most obvious with "git show" on your change. The README change has a trailing space on one line (shows read on the git show) and the TraceLoggingSupport.* files have \n endings instead of \r\n (VS's regex replace can fix that). I can't test it easily but it looks plausible.

randomascii commented 7 years ago

Uggh. I don't understand github. Or git for that matter. I merged your change in. I used a cherry-pick which has worked previously but which I'm probably being punished for. So naturally it thinks that it's a totally different change and that there is a conflict. Thinking it is a totally different change is probably because I committed a few changes after pulling yours into a branch. The conflicts are because I made some whitespace changes after cherry-picking yours.

But, it's all good. Your change is in. It seems to work? But give it a try and let me know. It will be in the next release.

And thanks!

mwinterb commented 7 years ago

Sorry for not addressing this before you had to deal with that. On "my end" they seemed to all be CRLF, but git show did point out the problems. Since I didn't know what was wrong, I was going to look at it some evening, but my last two nights were "booked". I should have mentioned that rather than just being silent.

But: testing with "my" two TraceLogging providers captures the right data when specifying them in the settings dialog. Thanks for the merge and fixing up those problems!

randomascii commented 7 years ago

No worries. It was trivial, just confusing when I saw the conflicts. But the change shows up with your name on it, and it sounds like it works, so that's all that matters.