trace2: remove inaccurate warning and add UI helper exit calls

[ldennington]

Remove warning about being unable to set up tracing from Trace2 TryParseSettings method. This method should just check to see whether TRACE2 is enabled - if it is not, it does not need to warn the user, as we only collect traces from those who actively choose to opt in.

Additionally, GCM's UI helpers are connected to the TRACE2 system via the Start() call in ApplicationBase.cs. Since this call records Version and Start events, ensure a corresponding Exit event is called before the helper process completes.

[mjcheetham]

A few questions on thread naming.

[mjcheetham]

Not sure if .NET Framework support the syntax sugar for ??=.

Also, does this mean if there's already a name we don't set it to main? Why not just always set it to main?

[ldennington]

I just used the suggestion from this documentation, but maybe that's being overly safe.

[mjcheetham]

Note that for helper UI apps we reserve the entry/primary thread for the UI dispatcher.

[jeffhostetler]

It is important that the main thread (the one sending the "version", "start" and "exit" events be called "main" so that I can special case other items that are associated with it.

are UI helper apps spawned as child processes from the "main" thread? that model works for me. Git's hook processes work that way. Really, all of Git creates children from the main thread -- threads are only really used for parallel algorithms IIRC.

[ldennington]

That is correct! Thanks for confirming. We will stick with this name, but I will remove the null check per @mjcheetham's comment.

[jeffhostetler]

This PR is just adding thread names to the existing version, start, and exit events, right?
We're not actually adding thread-start and thread-exit events to the mix yet, right?

If we assume that version, start, and exit will only be called on the main thread, you
could just hard code them for now in those events.

My concern is that later we need to think about C# thread usage -- lambdas and asyncs
and etc -- and how any tracing that is done from within a lambda body (or something it
calls) or in an async function needs to have a "logical" thread name based on the
activity, rather than a "physical" thread based upon the pool scheduling or whatever.
(I may be making this too complicated, but C# is doing some magic behind the scenes
that I need to know more about).

Then again, maybe we won't need to instrument GCM to that level, so it might not
matter. That is, if you're not running parallel algorithms or something, or are only
using asyncs to run child processes or something, maybe we don't need the thread-
level details, and everything could just be marked as being on "main". (sorry to be
so confusing)

[mjcheetham]

This PR is just adding thread names to the existing version, start, and exit events, right?

Yes.

We're not actually adding thread-start and thread-exit events to the mix yet, right?

Yes.

If we assume that version, start, and exit will only be called on the main thread, you
could just hard code them for now in those events.

The problem is we make heavy use of async/await so we're going to be running on all sorts of thread-pool threads, as well as the 'main' thread. I guess we can consider these to be 'fake' threads, and not care about them.

We don't actually spin up any 'real' threads explicitly(*), that I can remember off the top of my head. We only spin up child processes, which we're already handling.

(*) Caveat this with the fact for the UI helpers we really do explicitly spin up a new thread to run the 'main' application, and reserve the actual entry thread to run the UI dispatcher/message pump.

[ldennington]

(*) Caveat this with the fact for the UI helpers we really do explicitly spin up a new thread to run the 'main' application, and reserve the actual entry thread to run the UI dispatcher/message pump.

Based on what Jeff said above, I think we should probably call the UI Helper threads main as well (their names currently are set to AppMain). I'll make that fix as well as removing the null check. In terms of determining how to handle thread pool threads - since the thread names are all showing as either main or AppMain, I think we can save that determination for the next PR. But if I'm missing something, please let me know.

[ldennington]

This PR is just adding thread names to the existing version, start, and exit events, right?

@mjcheetham and I spoke offline and determined that the addition of thread names is going to require a bit more design than originally anticipated. It also may need to wait for the in-process UI helper work to land. I went ahead and removed that commit in light of this.

[jeffhostetler]

Let's not worry about the async pool threads right now. They only become significant if there are region-enter and -leave events being generated in the async proc / lamba body. If we're only interested in logging child process events, they can all just belong to "main".

and if the actual entry thread is just running the msg pump and you create a real thread to run the app, just let that new thread be "main" (assuming it will be the one doing all of the work and sending those 3 events).

[ldennington]

and if the actual entry thread is just running the msg pump and you create a real thread to run the app, just let that new thread be "main" (assuming it will be the one doing all of the work and sending those 3 events).

@mjcheetham and I discussed this yesterday and landed on not renaming this thread (naming it "main" could be misleading since it's not actually the main thread). I thought about it and, as a compromise for now, I'm going to just hardcode the Thread property to "main" with a TODO stating that this will need to change when we add regions.