Diagnostics: Not all dotnet-trace profiles should have Rundown and Sampling

[noahfalk]

I assume the current implementation of the 'None' profile still does Rundown at the end, and probably the gc profiles too. Rundown can be very time consuming and large so it may not be what the user wanted. Tasks:

• [ ] Figure out which of our hard coded profiles should have rundown included
• [x] Implement some mechanism that allows the trace client to configure rundown rather than having the runtime assume that rundown always happens.

[noahfalk]

@jorive - let me know if I misunderstood the current state of things, but I think last we talked this was still an outstanding issue. I'm trying to get everything tracked so we don't forget about work, regardless of how we ultimately prioritize it.

[jorive]

These are the "rundown" providers executed on Disable for all tracing sessions:

const EventPipeProviderConfiguration RundownProviders[] = {
    {W("Microsoft-Windows-DotNETRuntime"), 0x80020138, static_cast<unsigned int>(EventPipeEventLevel::Verbose), NULL},       // Public provider.
    {W("Microsoft-Windows-DotNETRuntimeRundown"), 0x80020138, static_cast<unsigned int>(EventPipeEventLevel::Verbose), NULL} // Rundown provider.
};

For more information: https://github.com/dotnet/coreclr/blob/master/src/vm/eventpipe.cpp#L402

[noahfalk]

Btw, if this is going to be hard and we need a quicker solution for 3.0, eliminating the profiles that shouldn't use Rundown is less good, but probably better than shipping them in a weird state.

[noahfalk]

@chrisnas reported seeing lots of sampling events when trying to do GC investigation - effectively a +1 for resolving this issue. It seems like this issue should be captured as a requirement of https://github.com/dotnet/diagnostics/issues/202 because the sampling events add non-trivial overhead which might prevent customers from being able to collect it in production.

[chrisnas]

In fact, after retesting everything it seems that I was wrong: if you use dotnet-trace --profile none --providers ..., the sampling provider is not enabled.
I've checked the execution under WinDBG and the sample profiler is not enabled by dotnet-trace.

I was lured by the fact that EventPipe::EnableInternal() is calling SampleProfiler::Initialize/SampleProfiler::Enable all the time (even when the sample provider is not set in dotnet-trace) but it is protected by the following test if (!s_pThreadTimeEvent->IsEnabled()) that returns true ONLY when the sample profiler is explicitly set.

However, I found another weird behaviour: if I'm enabling the sample provider with dotnet-trace, it is not disabled when dotnet-trace exits. I did not checked if the other providers:keyword:verbosity are also kept.

[jorive]

However, I found another weird behaviour: if I'm enabling the sample provider with dotnet-trace, it is not disabled when dotnet-trace exits. I did not checked if the other providers:keyword:verbosity are also kept.

I believe this is a known issue. Currently, we do not do a good job restoring enabled provider keywords once a session is disabled. We are working on it, and should have a PR out this week :)

[cshung]

In my opinion, making sure the sampler is proactively disabled is more important than avoiding rundown. Saving the rundown saves at most a few seconds per session, but saving the sampling profiler is going to save on average 10% overhead as I roughly measured.

[noahfalk]

making sure the sampler is proactively disabled is more important than avoiding rundown

I agree on the priority. Sung was going to confirm that it is being disabled correctly so I don't think we have to pick one or the other - we are getting both : D