Runtime: Add OSThreadId and TimeStamp to System.Diagnostics.Tracing.EventWrittenEventArgs

[brianrob]

As part of the .NET Core 2.2 performance monitoring work, I would like to add two new public APIs:

```c#
public sealed class EventWrittenEventArgs
{
///

/// Gets the identifier for the OS thread that wrote the event.
///

public long OSThreadId { get; }

/// <summary>
/// Gets a UTC DateTime that specifies when the event was written.
/// </summary>
public DateTime TimeStamp { get; }

}

## Rationale and Usage
As part of .NET Core 2.2, ```EventListener``` objects will be able to subscribe to native runtime events (e.g. GC, JIT, ThreadPool, etc.) in addition to events emitted by ```EventSource``` objects.  Each time an event is dispatched to an ```EventListener```, the ```OnEventWritten``` callback method is invoked passing an instance of ```EventWrittenEventArgs``` that contains all of the information associated with the event.

Historically, the thread ID and timestamp could be gathered from the environment by the callback itself because all ```EventSource``` events are dispatched synchronously on the same thread that emitted them.  
Native runtime events (at least some of them) cannot be dispatched synchronously because some events  such as the GC events are emitted when managed execution is suspended.  Thus, these events are buffered in native code and get dispatched by a dispatcher thread once managed code can execute again.  Because these events are buffered, the environment is not reliable when fetching the thread ID and timestamp - thus the need to expose them.

## Proposed APIs

```C#
    /// <summary>
    /// Gets the identifier for the OS thread that wrote the event.
    /// </summary>
    public long OSThreadId { get; }

The OSThreadId is used as a correlator to match two events that occurred on the same thread. This is a very common pattern in tracing log parsing. In addition to allowing for correlation between events in the process, it also allows for correlation between different logging systems and other performance tools that don't necessarily know about the managed thread ID concept, but are aware of OS threads. @vancem also has some explanation about how this is used here: https://github.com/dotnet/coreclr/pull/19002#discussion_r204504011.

```c#
///

/// Gets a UTC DateTime that specifies when the event was written.
///

public DateTime TimeStamp { get; }

The TimeStamp is used to determine when an event occurred.  Because events can now be buffered, such an API is needed in order to know when the event was actually created.

## Compatibility
There is existing customer code out in the wild that assumes that the thread ID and event creation time can be gleaned from the environment.  This is true for that code because there is no code out in the wild that consumes non-```EventSource``` events from within an ```EventListener```.

Consumption of these values from the environment will continue to work for this code and any future code that only subscribes to ```EventSource``` events.  Note that the consuming ```EventListener``` is the object that decides which events to subscribe to, and so the control of whether or not to subscribe to events that are buffered (and require these two new APIs) is fully controlled by the consumer, just like the code that either fetches the thread ID and timestamp from the environment or from these new APIs.

This makes subscription of the native events an opt-in for all ```EventListener``` objects, and makes the requirement to switch to using the two new proposed APIs also an opt-in operation.  Barring subscription to the newly exposed native events (an explicit action that must be taken), existing code will continue to work.

## Open Issues
One open issue that exists is that it is possible for an ```EventListener``` to subscribe to all events that exist.  This is done by implementing the ```EventListener.OnEventSourceCreated``` event.  This event is fired on creation of an ```EventSource``` and on creation of the ```EventListener``` for all existing ```EventSource``` objects, and allows the ```EventListener``` to subscribe to the events exposed by the ```EventSource``` using the following code pattern:

```c#
public class TestEventListener : EventListener
{
    // Called whenever an EventSource is created.
    protected override void OnEventSourceCreated(EventSource eventSource)
    {
        // Subscribe to all events exposed by the EventSource.
        this.EnableEvents(eventSource, EventLevel.Verbose, EventKeywords.All);
    }
}

The existing implementation of the performance monitoring feature causes a new framework-level EventSource (called RuntimeEventSource) that is part of System.Private.CoreLib to be created when an EventListener attempts to subscribe to native runtime events. This EventSource doesn't actually emit any events, but is used to make the native events look like managed EventSource events. When this EventSource is created, the OnEventSourceCreated event is fired for all existing EventListener objects, which could cause existing code to consume buffered events without knowing it, and thus get incorrect thread IDs and time stamps.

I recommend that we solve this by updating CoreCLR to not fire EventListener.OnEventSourceCreated for RuntimeEventSource, and thus make the use of these new events require an explicit opt-in.

Updates

• Changed the type of TimeStamp from DateTime to DateTimeOffset.
• Reverted the type change of TimeStamp back to DateTime per discussion below.

[brianrob]

FYI @vancem, @jkotas

[terrajobst]

Since this is slotted for .NET Core 2.2 we shouldn't wait until we have quorum, especially since the API surface is small. Two questions:

1. Are we sure long is the best representation for threads? What happens for cross-platform use? /cc @kouvel
2. Any reason we shouldn't use DateTimeOffset? We generally try to avoid DateTime in new APIs, if that's sensible/possible. /cc @tarekgh

/cc @dotnet/fxdc

[tarekgh]

Any reason we shouldn't use DateTimeOffset? We generally try to avoid DateTime in new APIs, if that's sensible/possible. /cc @tarekgh Tarek Mahmoud Sayed FTE

I prefer using dateTimeOffset too. DateTime can be ok if we ensure setting the DateTimeKind inside it as Utc but we can just avoid any zone confusion by returning DateTimeOffset instead.

[bartonjs]

I don't see long being inadequate. Is the concern just that it can way over-represent?

I agree on DateTime => DateTimeOffset, though.

[brianrob]

The choice of long is because OSX uses 64-bit thread IDs. (Windows and Linux are both 32-bit).

We do always create the DateTime as UTC, but I'm happy to change it to DateTimeOffset.

Edit: I've updated the proposal above with this change.

[kouvel]

The PAL uses size_t (pthread_threadid_np on OSX gets a 64-bit value), long should be fine

[vancem]

I think we have consensus that long is find for a threadID. Bascially anything that is big enough for a pointer (which long is) is fine. It was the cross-platform issue that pushed us from int to long (it was int until we discovered OSX uses something pointer sized).

I would like to push back on using DateTImeOffset. I realize that from an API point of view it is nicer, but the 'standard' for timestamps in telemetry is UTC which is what we will do here, and given that, DateTimeOffset adds no value but DOES make things twice as big (8 bytes -> 12 but rounds up to 16), and thus increase all manipuation costs. I will grant you that this cost is not huge, but we do tend to manipuate timestamps a fair bit (mostly to find time deltas), so the costs will probably be at least noticable.

Thus we have small but noticable cost, and zero value, which imples to me that we shoudl not do it.

As I said in telemetry everyone uses something that looks like UTC DateTime (a tick counter with a normalize time zone base). The logic I describe above (keep time zones out of this!, and the need for perf) is what drives everyone there), we shoudl just follow suit and use UTC DateTime.

[tarekgh]

I am ok to use DateTime with Utc kind. The point I was trying to say is when users have DateTime object (when they don't know the source of this object), they will have to be careful when using this object in any time calculations especially when time zones are involved. if this is not a concern in your scenario, so having DateTime will not be a problem.

[brianrob]

Based on @vancem and @tarekgh's discussion, I can revert the proposal back to using DateTime. Does anyone have any objections to this?

[brianrob]

I have updated the proposal below based on the discussion between @vancem and @tarekgh. Is there any other discussion that needs to happen here?

I have posted a PR for the contract change: dotnet/corefx#31564

[terrajobst]

Cool. Based on the conversation the API looks good as proposed then.

[brianrob]

Thanks much! I've merged this change.

[karelz]

Fixed in release/2.2 branch in PR dotnet/corefx#31703.