Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EventSource should mention some built-in types require special handling #2608

Closed
kevin-montrose opened this issue Sep 21, 2021 · 3 comments · Fixed by dotnet/dotnet-api-docs#7925
Assignees
Labels
documentation Documentation related issue
Milestone

Comments

@kevin-montrose
Copy link

kevin-montrose commented Sep 21, 2021

Documentation Request

EventSource does not mention that some built-in .NET types need to be mapped to the types expected by ETW. Presumably this has something to do with the reflection-generated manifests, but I haven't dug in to verify that.

To illustrate the issue, it is easy enough to modify some of the examples in the above link to end up with an event source like:

[EventSource(Name = nameof(CustomEventSource))]
internal class CustomEventSource : EventSource
{
    private const int MyEventId = 123;
	
    internal static readonly CustomEventSource Instance = new CustomEventSource();

    [Event(MyEventId, Version = 1)]
    public unsafe void MyEvent(int someInt, DateTime someDateTime, bool someBool)
    {
        Span<EventData> events = stackalloc EventData[3];
        events[0].DataPointer = (IntPtr)(&someInt);
        events[0].Size = sizeof(int);

        events[1].DataPointer = (IntPtr)(&someDateTime);
        events[1].Size = sizeof(DateTime);

        events[2].DataPointer = (IntPtr)(&someBool);
        events[2].Size = sizeof(bool);

        fixed (EventData* eventsPtr = events)
        {
            WriteEventCore(MyEventId, events.Length, eventsPtr);
        }
    }
}

Which is subtly wrong as DateTime and bool do not map 1-to-1 to the format ETW expects - specifically DateTime must be a FILETIME and bool must be mapped to an int of 1 or 0. bool is particular insidious, as the size mismatch between the passed and expected types can result in data corruption (for the following EventDatas) and potentially reads garbage data off the stack.

A corrected version of the above would be:

[EventSource(Name = nameof(CustomEventSource))]
internal class CustomEventSource : EventSource
{
    private const int MyEventId = 123;
	
    internal static readonly CustomEventSource Instance = new CustomEventSource();

    [Event(MyEventId, Version = 1)]
    public unsafe void MyEvent(int someInt, DateTime someDateTime, bool someBool)
    {
        Span<EventData> events = stackalloc EventData[3];
        events[0].DataPointer = (IntPtr)(&someInt);
        events[0].Size = sizeof(int);

        var someDateTimeAsFileTime = someDateTime.ToFileTimeUtc();
        events[1].DataPointer = (IntPtr)(&someDateTimeAsFileTime);
        events[1].Size = sizeof(long);

        var someBoolAsInt = someBool ? 1 : 0;
        events[2].DataPointer = (IntPtr)(&someBoolAsInt);
        events[2].Size = sizeof(int);

        fixed (EventData* eventsPtr = events)
        {
            WriteEventCore(MyEventId, events.Length, eventsPtr);
        }
    }
}

Ideally, any built-in System.* types that might reasonably be on an event should have an example of how to prepare them for EventData. I don't know how involved that would be, so an acceptable alternative would be to just call out that some types need special handling and a link to the relevant ETW documentation (which might be this).

Previous documentation

There's the existing documentation of the EventSource class which should probably be amended.

There's also ETW's input type remarks which are relevant, though the mapping from those types to .NET types is not always clear.

@kevin-montrose kevin-montrose added the documentation Documentation related issue label Sep 21, 2021
@kevin-montrose
Copy link
Author

@hoyosjs ^ ping, per Vinod

@KalleOlaviNiemitalo
Copy link

For C# 9, a source generator could generate the bodies of these methods and take care of the marshalling. dotnet/runtime#45699 already added EventSourceGenerator but it does not do this yet, as it focuses on initialization performance rather than developer convenience. The documentation would still help developers using other languages or .NET Framework targets that do not support C# 9.

@tommcdon tommcdon added this to the 7.0.0 milestone Nov 8, 2021
@noahfalk noahfalk self-assigned this Mar 8, 2022
noahfalk added a commit to noahfalk/dotnet-api-docs that referenced this issue Apr 5, 2022
Fixes: dotnet/diagnostics#2608

WriteEventCore takes an array of EventData arguments but only provided a few
sparse examples on how to initialize these values. Several of the types not shown
have non-intuitive encodings that can't be inferred from the few examples. This
change provides a more complete listing, in particular including bool, DateTime,
string, and byte[] which are non-obvious.
noahfalk added a commit to noahfalk/dotnet-api-docs that referenced this issue Apr 5, 2022
Fixes: dotnet/diagnostics#2608

WriteEventCore takes an array of EventData arguments but only provided a few
sparse examples on how to initialize these values. Several of the types not shown
have non-intuitive encodings that can't be inferred from the few examples. This
change provides a more complete listing, in particular including bool, DateTime,
string, and byte[] which are non-obvious.
@noahfalk
Copy link
Member

noahfalk commented Apr 8, 2022

Fixed! dotnet/dotnet-api-docs#7925

@noahfalk noahfalk closed this as completed Apr 8, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Documentation related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants