Indicate that this is optional and what the file is if you don't specify it.

Fixed.

Tell them what the defaults are (CPU sampling).

Fixed.

Indicate that this has to be done before the program starts, but has no perf impact until the control file exists.

Fixed.

Presumably the logic that turns things on if the value is 1 is in native code. Any chance we can plumb things so that we move that here? Ideally all 'high level' control logic is managed. Certainly it is strange that it is split.

Yes, this can be done, and is a good thing to do. I've fixed this.

Ideally all of this just gets pushed to managed code and it is there where config values are fetched and set as needed. Native code should not know about config at all.

Yup, done.

Perhaps int.TryParse would be better than Convert.ToInt32? I know that we have a catch block here, but we could avoid the first chance exception here

It turns out that Convert.ToInt32 will return 0 if the input string is null.

But it's going to throw when the string will be incorrect, example: Convert.ToInt32("not a number");

This is true. I've switched to TryParse. Thanks for pointing this out.

I would simplify the method and reduce nesting to:

internal static void Initialize()
{
    if (s_controllerInstance != null)
        return;

    try
    {
        if(int.TryParse(CompatibilitySwitch.GetValueInternal("EnableEventPipe"), out int flag) && flag == Constants.EventPipeInitiallyDisabled)
            s_controllerInstance = new EventPipeController();
    }
    catch {}    
}

Also I would check if CompatibilitySwitch.GetValueInternal can throw. If not, I would remove the catch block.

I want to leave the try/catch block here to be explicit and to be a reminder in the future that this must not throw.

Thread.Sleep(Timespan.FromSeconds(10)) would be more self-describing

You're right - the reason I didn't do this is because TimeSpan.FromSeconds has a higher cost than just using a const int, and then it just has to be converted to an integer.

👍 I like that you mention the unit in both variable name and the description!

Thanks!

While polling every 10 seconds to enable makes sense to keep the overhead very low, it does not make sense for disable . It would be useful to change to to 1 sec when enabled like this.

 Thread.Sleep(PollingIntervalMilliseconds);

to

 Thread.Sleep(isEnabled ? 1 : PollingIntervalMilliseconds);

Agreed. Fixed.

You should not need private provider or the sample profiling provider to do a heap dump.
You SHOULD just need the keyword 0x1180109 at informational (4) level. (Please test it). See https://github.com/Microsoft/perfview/blob/master/src/EtwHeapDump/DotNetHeapDumper.cs#L47

We should consider making shorcuts for common configurations. For example we could make $ be special config name marker and

export COMPlus_EventPipeConfig=$HeapDump

means

export COMPlus_EventPipeConfig=Microsoft-Windows-DotNETRuntime:0x1180109

What I want is for it to be at least conceivable that you could do this without a script or cut/paste.

Alternatively you could make the number used for COMPlus_EnableEventPipe indicate the default config.

Let me think about how best to do this. I suspect COMPlus_EventPipeConfig=$HeapDump would probably be the right way. We can also have $GCCollect and $GCCollectOnly as well.

Note that there is a bit of an ugly problem if you end up running more than one .NET Process because this environment variable will be used by each process. At best you lose the ability to collect data on the other processes (somebody wins).

I would recommend changing things so that if the target file exists, you morph the name to .PID.netperf (roughly what you do by default). In the 'normal' case they get what they expected, but in the unusual case data is not lost.

Fixed. The control file name won't change - just the trace file itself (and only if the requested name already exists).

Indicate at least loosely what is in the default (includes CPU/THreadtime sampling, and .NET runtime events including GC, JIT, Exception and ThreadPool)

Done.

There is a way to get the processID in managed code in system.private.corelib

Win32Native.GetCurrentProcessId() 

EventSource already uses this. You should use this and generate the file name here, rather than the pain of creating an FCALL to do it on the native side.

Win32Native.GetCurrentProcessId() is a Windows-specific pinvoke:

        [DllImport(Interop.Libraries.Kernel32, CharSet = CharSet.Auto, SetLastError = true)]
        internal static extern uint GetCurrentProcessId();

If I remember correctly, this is used for some ETW-specific features.

If desired, we can expose the process ID via FCALL and do the work in managed code.

I think we'll need to just plumb the PID to managed code to use it to resolve filename conflicts.

Hmm, I saw that that definition was windows specific, but what I also know is that the call site for Win32Native.GetCurrentProcessId() is unconditional (EnsureDescriptorsInitialized has to be called even on Linux (or eventListeners don't work) and there are no ifdefs around the call to GetCurrentProcessId.

I was assuming on Linux we created look-alikes, but frankly I don't know how it works, so I could be wrong. Still just try it and testing will show that it works on Linux (or you have to plumb through the FCALL).

I agree this is windows-specific code is no longer needed (we can do something that is naturally cross-platform).
Have we tested that the new heap dump works the same as the old (if we have a heap dump test, that would do it)

I have not written any specific tests for this, but I have done some adhoc validation that looks good so far.

We can avoid burning the thread by

 m_timer = new Timer(PollTracingCommand, null, 10000, 10000);

Which achieves the same effect but does not uses threadpool threads (and only for very short durations).

PollTracingCommand is now one iteration of the loop you have below.

I did check and on Linux Win32Native.GetCurrentProcessId() doe exist. What happens is on linux Interop.Libraries.Kernel32 is redirected to "libcoreclr", and these methods are implemented in libcoreclr.so.

Here is a list of methods that are exported in libcoreclr.so (nm -D libcoreclr.so)

00000000004e2c10 T CloseHandle
00000000000ccdf0 T coreclr_create_delegate
00000000000ccf00 T coreclr_execute_assembly
00000000000cc8f0 T coreclr_initialize
00000000000cccf0 T coreclr_shutdown
00000000000ccd70 T coreclr_shutdown_2
00000000000cc180 T CoreDllMain
0000000000524fa0 T CoTaskMemAlloc
0000000000524fc0 T CoTaskMemFree
0000000000524fb0 T CoTaskMemRealloc
00000000004ffb20 T CreateEventExW
00000000004ffa90 T CreateEventW
00000000005008b0 T CreateMutexExW
00000000005007b0 T CreateMutexW
00000000005001c0 T CreateSemaphoreExW
0000000000500250 T CreateSemaphoreW
00000000000cc280 T DllMain
00000000004e2960 T DuplicateHandle
00000000004ee9c0 T FormatMessageW
00000000004ee250 T FreeEnvironmentStringsW
00000000004e5c80 T GetACP
00000000000f4210 T GetCLRRuntimeHost
0000000000509f80 T GetCurrentProcessId
000000000050e780 T GetCurrentThreadId
00000000004edfe0 T GetEnvironmentStringsW
00000000004edbf0 T GetEnvironmentVariableW
00000000000cc3e0 T GetMetaDataInternalInterface
00000000000cc3f0 T GetMetaDataInternalInterfaceFromPublic
00000000000cc400 T GetMetaDataPublicInterfaceFromInternal
00000000004e4640 T GetProcAddress
00000000004dccd0 T GetStdHandle
00000000004f0190 T GetSystemInfo
00000000004f01f0 T GlobalMemoryStatusEx
00000000004ebff0 T LocalAlloc
00000000004ec080 T LocalFree
00000000004ec040 T LocalReAlloc
00000000004ce000 T lstrlenA
00000000004ce020 T lstrlenW
00000000000cc2e0 T MetaDataGetDispenser
00000000004e5e90 T MultiByteToWideChar
00000000004ffe70 T OpenEventW
0000000000500d80 T OpenMutexW
0000000000500470 T OpenSemaphoreW
00000000004d4dc0 T OutputDebugStringW
00000000004e5230 T PAL_RegisterModule
00000000004e53f0 T PAL_UnregisterModule
00000000004f06b0 T QueryPerformanceCounter
00000000004f0710 T QueryPerformanceFrequency
00000000004d5da0 T RaiseException
00000000005008e0 T ReleaseMutex
00000000005002e0 T ReleaseSemaphore
00000000004ffd80 T ResetEvent
00000000004edd70 T SetEnvironmentVariableW
00000000004ffbc0 T SetEvent
00000000005243c0 T SysAllocStringLen
0000000000524570 T SysFreeString
00000000005245b0 T SysStringLen
00000000004ea840 T VirtualAlloc
00000000004eb3f0 T VirtualFree
00000000004eb9d0 T VirtualQuery
00000000004e5f20 T WideCharToMultiByte
00000000004dc9f0 T WriteFile

Notice that GetCurrentProcessId is one of them.

Thus I don't believe you need create the QCALL for it, just use Win32Native.GetCurrentProcessId.

I have switched to this and will confirm that it works on Linux.

I have confirmed that you are correct, and this does work on Linux.

I am including this again, because it may have been lost inadvertantly.

We can avoid burning the thread by

 m_timer = new Timer(PollTracingCommand, null, 10000, 10000);

WHere PollForTracingCommand is now one iteration of the loop in WaitForTracingCommand.

This achieves the same effect but does not tie up a thread for the duration of the process.

Yes, this was lost inadvertently. I have switched to a timer. Please take a look at the implementation. I've set it up to avoid concurrent polling operations by scheduling the next poll operation during the callback. I've done some measurement and have not seen this show up, but of course, this is with a sampling profiler. Let me know if you have any concerns with this.

I have no concern with scheduling the next timer in the callback. A 10 second interval insures that the cost will be << .1%.