[API Proposal]: Add `EventSourceEventGenerateAttribute` to generate event code

[Cricle]

Background and motivation

According to eventsource-getting-started

Eventone must call like WriteEvent or WriteEventCore to write event.

But this is a regular code.

API Proposal

No any changed, but the partial base on EventSource class, partial method and has EventAttribute method will be generated.

API Usage

internal sealed unsafe /*It's mandatory or in method write unsafe*/ partial class RuntimeEventSource : EventSource
{
    [Event(1)]
    public partial void GoHome(string address,double usedTime);
}

//Will generated
public void GoHome(string address,double usedTime)
{
    address ??= System.String.Empty;
    global::System.Diagnostics.Tracing.EventSource.EventData* datas = stackalloc global::System.Diagnostics.Tracing.EventSource.EventData[2];

    fixed(char* addressPtr = address)
    {
        datas[0] = new global::System.Diagnostics.Tracing.EventSource.EventData
        {
            DataPointer = addressPtr,
            Size = (address.Length + 1) * sizeof(char)
        };
        datas[1] = new global::System.Diagnostics.Tracing.EventSource.EventData
        {
            DataPointer = (nint)(&usedTime),
            Size = sizeof(double)
        };     
        WriteEventWithRelatedActivityIdCore(1, null, 2, datas);
    }
}

Alternative Designs

No

Risks

No

[julealgon]

ActivitySource is mentioned in the title but not in the post. Can you elaborate what the benefits would be for that?

[Cricle]

ActivitySource is mentioned in the title but not in the post. Can you elaborate what the benefits would be for that?

Oh！I forgot write that 😱

[tommcdon]

Hi @Cricle! Some source generator work has been done on EventSource, for example https://github.com/dotnet/runtime/pull/45699. Remaining work is tracked on https://github.com/dotnet/runtime/issues/56154. Our team does not have immediate plans to work on this - please do feel free to submit a PR if you plan on working on it as we welcome community contributions!

[Cricle]

I think this will be a huge challenge for me.

But I will try to do it.

I am not familiar with the code structure of runtime, so I may not be able to complete it in 10.0.0.

[Cricle]

But in reality, the form of ActivitySource event generation is still unknown

[jkotas]

    //Use unsafe method because it is faster than fixed, and easily to generate
   DataPointer = (nint)global::System.Runtime.CompilerServices.Unsafe.AsPointer(ref global::System.Runtime.InteropServices.MemoryMarshal.GetReference(global::System.MemoryExtensions.AsSpan(address)),

This is invalid use of AsPointer. It would introduce intermittent crashes. From https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.unsafe.aspointer: "If the original managed pointer points within a GC-managed object, the caller must ensure that the source object is pinned". You are not doing that here.

[Cricle]

    //Use unsafe method because it is faster than fixed, and easily to generate
   DataPointer = (nint)global::System.Runtime.CompilerServices.Unsafe.AsPointer(ref global::System.Runtime.InteropServices.MemoryMarshal.GetReference(global::System.MemoryExtensions.AsSpan(address)),

This is invalid use of AsPointer. It would introduce intermittent crashes. From https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.unsafe.aspointer: "If the original managed pointer points within a GC-managed object, the caller must ensure that the source object is pinned". You are not doing that here.

Do I change to 'fixed' for the manage object？

[jkotas]

Do I change to 'fixed' for the manage object？

Right

[Cricle]

Do I change to 'fixed' for the manage object？

Right

Done to change it

[noahfalk]

Thanks @Cricle! I started looking through this and a couple questions so far:

1. The attribute you are proposing is named 'GeneratedEventSourceEventAttribute' but the attribute in the proposed usage is 'EventSourceEventGenerate'. Are these two attributes intended to be the same or am I misunderstanding something?

2. Your example has a partial OnRaise(int,string) method, but doesn't show an implementation so I wouldn't expect the code to compile as-is. What is the goal for this OnRaise() method and who is intended to implement it?

3. The OnRaise method has a parameter signature that is int, string but I'm not sure how the generator would determine this parameter list?

@jaredpar - I don't have a lot of expertise and what is easy/hard to implement in a source generator so I was hoping to get your advice on a design question here.

My hope is that we wouldn't need a new attribute that drives the source generator and instead it could activate automatically whenever the implementation for one of these EventSource [Event] attributed methods is missing.  Would it be challenging for the source generator to determine the set of partial methods with that attribute for which no implementation is present? Are there any other problems you are aware of trying to use that approach?

Example EventSource:

[EventSource(Name="Foo")]
partial class FooEventSource
{
    // this method already has an impl so the source generator shouldn't create one.
    [Event(1)]
    public partial void AppStarted(string message, int favoriteNumber) => WriteEvent(1, message, favoriteNumber);

    // this method is missing an impl and needs an auto-generated implementation
    [Event(2)]
    public partial void AppStopped(string message);
}

Although I wouldn't expect it is common, I'd also hope the generator could be robust in the presence of extra partial definitions:

partial class FooEventSource
{
    // Even though the method doesn't have an impl here, the generator would need to recognize the previous
    // partial definition site above does provide an impl so it avoids creating a duplicate one.
    [Event(1)]
    public partial void AppStarted(string message, int favoriteNumber);
    [Event(2)]
    public partial void AppStopped(string message);
}

[jaredpar]

@333fred, @CyrusNajmabadi

[Cricle]

@noahfalk Thanks for discussing

1. The new attribute I think can be removed, as your hope say. Less change is the best.

2. I think generate OnXXX and the parameter same as event, can provide to Meter and Counter. The parameter list generate has been implemented.

You can see my project Diagnostics.Net

The issure content will be change later.

[noahfalk]

I think generate OnXXX and the parameter same as event, can provide to Meter and Counter

The way you have it written above, they don't match:

public void GoHome(string address,double usedTime)
partial void OnRaise(int i, string address)

If I assume they were supposed to match that would be OnGoHome(string address, double usedTime) right?

If the developer would be responsible for writing OnGoHome() I think it might be easier if we let the developer create and name their own wrapper rather than asking them to learn about a specific callback naming convention the generator is using. For example the developer could write:

public MyEventSource : EventSource
{
    [NonEvent]
    public HandleGoHome(string address, double usedTime)
    {
        // do some other custom work first. This can have whatever the dev would have written in OnGoHome()
        AddCounter();

        // write the EventSource event
        GoHome(address, usedTime);
    }

    [Event(1)]
    partial void GoHome(string address,double usedTime);
}

If the generator is automatically implementing OnGoHome() via yet more attributes similar to your Diagnostics.Net project, that may be code-gen better left to external source-generator rather than something bundled with the runtime. I don't know that there would be much demand for it so I'd prefer not to have the runtime team become responsible for maintaining it. I believe there are mechanisms for NuGet packages to disable a built-in source generator in favor of a custom one but I'd need to hunt down the details.

[Cricle]

Right, it is a written error.

The difference between these two lies in who is responsible for maintenance and the issue of code volume.

The special situation may be relatively rate.

I will change the issue content later

[Cricle]

@noahfalk @jaredpar I still want to add attribute to control it.

Please roslyn team join in the discussion.

[jaredpar]

My hope is that we wouldn't need a new attribute that drives the source generator and instead it could activate automatically whenever the implementation for one of these EventSource [Event] attributed methods is missing.

The ideal state is having a source generator that triggers off an attribute. There is a bit of work to determine if the method is missing or not but that is doable.

[noahfalk]

The ideal state is having a source generator that triggers off an attribute

Thanks! When you say an attribute, I assume you mean a new attribute whose only purpose is to activate the source generator as opposed to the pre-existing Event attribute?

@noahfalk @jaredpar I still want to add attribute to control it.

If we do use an attribute, what do you think about having it be per-method rather than per-type? I'm anticipating that there is a fair amount of history where developers have been writing custom EventSource method implementations and it may be more awkward to adopt the generator if it is an all or nothing choice at the class level.

[Cricle]

Same as LoggerMessage generator?

In history EventSource the Event method does not write partial keyword, because it must be implemented.

The class level means: tag class, write your events and use partial keyword, the generator will do it.

The method level means: tag all what you want to be generated methods, use partial keyword, the generator will do it.

The difference with class level no need to write many control attributes, but the method level has higher precision in control.

For me, I prefer to write less, what about you.

[noahfalk]

For me, I prefer to write less, what about you.

I'm not opposed to having a class level option, but I'd be skeptical if it is the only option because anyone who wants to customize a single method would have to abandon the auto generation completely.

[Cricle]

Do you mean the user can select the class or method control?

[noahfalk]

Yeah, I mean the user could write either:

[AutoGenerate] // TBD what the actual name of this attribute should be
[EventSource(Name="Foo")]
public class FooEventSource
{
    [Event(1)]
    public partial void GoHome(string address,double usedTime);
    [Event(2)]
    public partial void SomeOtherEvent(string arg);
}

OR

[EventSource(Name="Foo")]
public class FooEventSource
{
    [Event(1)]
    [AutoGenerate]
    public partial void GoHome(string address,double usedTime);
    [Event(2)]
    [AutoGenerate]
    public partial void SomeOtherEvent(string arg);
}

As a heads up, I'll be on an extended vacation starting at the end of this week and I don't know if anyone else will be available to discuss the design. Unfortunately it may need to pause until January unless someone else can take over.

[Cricle]

I think this is feasible and practical, do others have any other opinions?