[API Proposal]: Add overload for EventSource primitives

[agocke]

Summary

I've seen a lot of calls to the varargs EventSource overload that have to manually suppress the trim warning because they only use primitive types.

This PR adds an overload that only supports primitive types, and thus will always be trim-compatible.

Adding this overload solves two problems:

1. Users won't get a warning if their usage is safe.
2. Users won't have to suppress a warning.

Proposed API

        protected void WriteEvent(int eventId, params EventSourcePrimitive[] args) { }

        public readonly struct EventSourcePrimitive
        {
            public static implicit operator EventSourcePrimitive(bool value) => ...;
            public static implicit operator EventSourcePrimitive(byte value) => ...;
            public static implicit operator EventSourcePrimitive(short value) => ...;
            public static implicit operator EventSourcePrimitive(int value) => ...;
            public static implicit operator EventSourcePrimitive(long value) => ...;
            public static implicit operator EventSourcePrimitive(string? value) => ...;
        }

This API exploits C# implicit conversion and overload resolution rules to prefer binding to an EventSourcePrimitive when the input type is one of the implicitly converted types.

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

Issue Details

---

I've seen a lot of calls to the varargs EventSource overload that have to manually suppress the trim warning because they only use primitive types.

Adding this overload solves two problems:

1. Users won't get a warning if their usage is safe.
2. Users won't have to suppress a warning.

Author:	agocke
Assignees:	agocke
Labels:	area-System.Diagnostics.Tracing, new-api-needs-documentation
Milestone:	-

[eerhardt]

Doesn't this need to follow https://github.com/dotnet/runtime/blob/main/docs/project/api-review-process.md?

[agocke]

@eerhardt Happy to go through that as well. I wanted to get some early feedback on whether this would address areas you've seen. Turns out, I found a bunch of places in the framework that it solves, so maybe that answered my question.

[noahfalk]

I take it C# overload resolution rules bind params EventSourcePrimitive[] with higher priority than params object[] when all the parameter types have implicit casting rules defined?

How many warnings does this handle? My guess is that if the number of sites is measured in hundreds then it could be worth adding the API, but if it is <100 we should probably just annotate them.

The API itself looked fine to me assuming a final version includes all the other things that go along with new APIs like tests and docs.

[noahfalk]

@davmason

[davmason]

I don't have a warm fuzzy feeling about adding new APIs just to make the trimmer happy. Is it possible to make emitting the warning smarter? It seems like it wouldn't be that hard to check if it's just primitives then don't warn, but I haven't thought super hard about it.

[agocke]

@davmason Adding new APIs when the originals are unsafe is the design recommendation.

[agocke]

I take it C# overload resolution rules bind params EventSourcePrimitive[] with higher priority than params object[] when all the parameter types have implicit casting rules defined?

Yup

How many warnings does this handle? My guess is that if the number of sites is measured in hundreds then it could be worth adding the API, but if it is <100 we should probably just annotate them.

I've hit a warning from this in almost every third-party library I've tried to annotate, so this is likely worth it just because users wouldn't have to add suppressions (which we explicitly recommend against).

The API itself looked fine to me assuming a final version includes all the other things that go along with new APIs like tests and docs.

Yeah, I could use some guidance on where the tests live. This may actually be inadvertently testing some old code paths, since they preferentially bind to this one now.

[eerhardt]

I could use some guidance on where the tests live

You can write trimming tests (small .exe apps that get full trimmed and then executed - ensuring they return exit code 100) here:
https://github.com/dotnet/runtime/tree/main/src/libraries/System.Diagnostics.Tracing/tests/TrimmingTests

[agocke]

@eerhardt Given that we're compiling libraries tests with AOT now, is that the preferred path? Or is there another reason for the separation?

[eerhardt]

The reason for the separation is the same as it always has been: When you are unit testing, your goal is exercise all the code in the code you are testing. Which means very little of it gets trimmed away (if it DOES get trimmed it means you have a test hole).

The point of the trimming tests is to use the API under test in an isolated manner, with no other code being rooted in the app.

See https://github.com/dotnet/designs/blob/main/accepted/2020/linking-libraries.md#testing for more info.

[bartonjs]

Video

• This type should support all of the types that EventSource considers a primitive.
• If any of the specific types are a trimming problem then the conversion operator can/will gain the trimmer warning attributes.
• nuint is in the list to force it to explicitly go through ToString instead of an implicit ulong conversion.
• Passing a literal null into WriteEvent(int, params object[])/WriteEvent(int, params EventSourcePrimitive[]) will become a source breaking change, but we're OK with that.
• Tracing may want to look into adding support for additional types (e.g. TimeOnly/DateOnly/DateTimeOffSet/Half/Int128/UInt128/nuint), all of which currently go through ToString(). This proposal merely pointed out the strangeness at this boundary.

namespace System.Diagnostics.Tracing
{
    public class EventSource : IDisposable
    {
        protected void WriteEvent(int eventId, params EventSourcePrimitive[] args) { }

        public readonly struct EventSourcePrimitive
        {
            private object _reference;
            private int _value;

            public static implicit operator EventSourcePrimitive(bool value) => ...;
            public static implicit operator EventSourcePrimitive(byte value) => ...;
            public static implicit operator EventSourcePrimitive(short value) => ...;
            public static implicit operator EventSourcePrimitive(int value) => ...;
            public static implicit operator EventSourcePrimitive(long value) => ...;

            public static implicit operator EventSourcePrimitive(sbyte value) => ...;
            public static implicit operator EventSourcePrimitive(ushort value) => ...;
            public static implicit operator EventSourcePrimitive(uint value) => ...;
            public static implicit operator EventSourcePrimitive(ulong value) => ...;

            public static implicit operator EventSourcePrimitive(float value) => ...;
            public static implicit operator EventSourcePrimitive(double value) => ...;
            public static implicit operator EventSourcePrimitive(decimal value) => ...;

            public static implicit operator EventSourcePrimitive(string? value) => ...;
            public static implicit operator EventSourcePrimitive(byte[]? value) => ...;

            public static implicit operator EventSourcePrimitive(Guid value) => ...;
            public static implicit operator EventSourcePrimitive(DateTime value) => ...;
            public static implicit operator EventSourcePrimitive(nint value) => ...;
            public static implicit operator EventSourcePrimitive(nuint value) => ...;
            public static implicit operator EventSourcePrimitive(char value) => ...;

            public static implicit operator EventSourcePrimitive(Enum value) => ...;
        }
    }
}

[davidfowl]

@JeremyKuhne this feels like your variant proposal from a while back.

[JeremyKuhne]

@davidfowl yes, it does seem very similar. See #28882. I have a fully working prototype that Azure is already using internally. https://github.com/JeremyKuhne/ValuePrototype

It would be nice to have a general purpose struct for this sort of stuff.

[JeremyKuhne]

One notable difference here is that my proposal has an additional type that EventSource would not be able to handle (DateTimeOffset). Presumably that could just throw an ArgumentException?

[agocke]

I've deliberately left some options open here for optimizations. Also noted is some deficiencies in the overall API support

TimeOnly/DateOnly/DateTimeOffSet/Half/Int128/UInt128/nuint

Those things would have to be addressed in a future evolution.

[agocke]

@davmason I think this is ready to merge. Can you take a look?

[davmason]

LGTM, thanks!

[tommcdon]

@agocke are there any outstanding work items that need to be completed or is this PR ready to merge?

[agocke]

@tommcdon Two failing tests: CoreCLR Linux x64 Debug and Mono Linux x64 Debug are both timing out. Trying to figure out why.

[agocke]

Failures are unrelated -- also happening in main.