[API Proposal]: Introduce source generation based support for .NET metrics API

[dpk83]

Background and motivation

Our team creates Telemetry framework for internal teams and we have created metrics solution. It was created before .NET Meter API came into existence so we created our own IMeter API surface. In our internal project, everything is designed to be highly efficient (consumes less CPU and reducing allocation as much as possible) while providing an easy to use developer friendly API surface. This helps reduce the developer burden, reduces chances of mistakes and at the same time helps gain the performance benefits.

With that in mind we utilized the compile time source generation of roslyn and introduced attributes for various instruments. Here is an example of how the attributes look like in our internal project using our IMeter surface

[Counter<int>("clientRing", "clientType", "region", Name = "TestInstrument")]
public static partial TestInstrument CreateTestInstrument(this IMeter meter); 

Developer writes this signature using Counter attribute and we would generate the source code for this method underneath which generates a highly efficient source using the most efficient mechanism available to achieve the required operations.

Once the above signature is defined, developers can then use it to create the typed counter like this:

// Create the instrument
var testInstrument = meter.CreateTestInstrument();

// The generated source will method generated specifically for this instrument i.e. it will have a method which will have named 
// parameters with names defined in the Counter attribute definition. This makes it easier for a developer to know what dimensions are expected to be passed and can provide it appropriately
// Use it to record metrics wherever needed. 
testInstrument.Add(1, clientRing, clientType, region);

Similarly, these attributes also support strong typed objects e.g.

[Counter<long>(typeof(MyObject), Name="MyStrongTypedInstrument")
public static partial MyCounterInstrument CreateMyCounterInstrument(this IMeter meter);

You can then use it as

var counter = meter.MyCounterInstrument();

counter.Add(10, myObject);

The underlying code generated at the compile time would expand this object's properties into individual dimension, thus simplifying the work for developers.

We are extending this source generation to support .NET Meter API and would like to have this become part of .NET so it can be useful for the broader community.

With the code generation, we have possibilities to provide the highest performance. E.g. we could automatically use the TagList if the total tags are less than or equal to 8 and switch to expanded tag list when there are more. We can skip all the processing upfront if there are no listeners or the instrument is not enabled etc.

We have found this extremely useful and so did our customers. We can build additional functionality on top where we could add a source generator to generate reports of the metrics emitted by a service and all it's dependent libraries (so developers know upfront what metrics can be emitted by libraries that they depend on, which ones are enabled vs which ones are not etc.)

API Proposal

[AttributeUsage(AttributeTargets.Method)]
[Conditional("CODE_GENERATION_ATTRIBUTES")]
public sealed class CounterAttribute<T> : Attribute
{
    public CounterAttribute(params string[] dimensions)
    {
        Dimensions = dimensions;
    }

    public CounterAttribute(Type type)
    {
        Type = type;
    }

    public string? Name { get; set; }
    public string[]? Dimensions { get; }
    public Type? Type { get; }
}

Similarly for HistogramAttribute

API Usage

// Create the signature at some place
[Counter<int>("clientRing", "clientType", "region", Name = "TestInstrument")]
public static partial TestInstrument CreateTestInstrument(this IMeter meter); 


// Create the instrument, where needed 
var testInstrument = meter.CreateTestInstrument();

// Use it to record metrics when needed. 
testInstrument.Add(1, clientRing, clientType, region);

Similarly, these attributes also support strong typed objects e.g.

[Counter<long>(typeof(MyObject), Name="MyStrongTypedInstrument")
public static partial MyCounterInstrument CreateMyCounterInstrument(this IMeter meter);

// Create the instrument using the declared method 
var counter = meter.MyCounterInstrument();

// Record the metric and pass the object when needed
counter.Add(10, myObject);

Alternative Designs

No response

Risks

No response

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[dpk83]

@noahfalk @tarekgh

[noahfalk]

@dpk83 - high level I like the idea of source generation for metrics similar to what exists for logging. I'm curious if you've considered or gotten feedback on designs where the source compiled APIs directly call the instrument APIs vs. returning the instrument to the user and requiring them to make the call? Here is what I imagine usage of the two different styles looks like:

Static factories:

static class MyHomeControllerMetrics
{
    [Counter<long>(typeof(MyObject), Name="MyStrongTypedInstrument")
    public static partial MyCounterInstrument CreateMyCounterInstrument(this IMeter meter);
    [Counter<int>("clientRing", "clientType", "region", Name = "TestInstrument")]
    public static partial TestInstrument CreateTestInstrument(this IMeter meter);
} 

class MyHomeController
{
    MyCounterInstrument _counter;
    TestInstrument _test;
    // ... additional instruments listed here

    public MyHomeController()
    {
        IMeter meter = ...
        _counter = MyHomeControllerMetrics.CreateMyCounterInstrument(meter);
        _test = MyHomeControllerMetrics.CreateTestInstrument(meter);
        // ... additional instruments inited here
    }

    public void DoSomeWork(MyObject obj)
    {
        _counter.Add(10, obj);
    }
}

Direct counter manipulation:

partial class MyHomeControllerMetrics
{
    [Counter(Name="MyStrongTypedInstrument")]
    public partial void AddCounter(long increment, MyObject obj);
    [Counter(Name="TestInstrument")]
    public partial void AddTest(int increment, string clientRing, string clientType, string region);
} 

class MyHomeController
{
    MyHomeControllerMetrics _metrics;

    public MyHomeController()
    {
        IMeter meter = ...
        _metrics = new MyHomeControllerMetrics(meter);
    }

    public void DoSomeWork(MyObject obj)
    {
        _metrics.AddCounter(10, obj);
    }
}

I find the second one a little more appealing for a few reasons:

• simplifies the attributes by inferring most of the information from the partial method signature
• doesn't require generating as much code (1 method/instrument vs. 2 methods and 1 type/counter)
• doesn't require the user to cache each instrument locally saves 2 LoC per instrument after the first

If there are situations that require creating or accessing the instrument I'm not opposed to it, but it didn't seem as streamlined for common usage.

Although out-of-scope here, I'm also imagining further down a roadmap we might use source generation to help streamline emitting multiple types of telemetry without user code needing to cache different factory objects in fields for producing each kind of data they want to send:

// this type could be wrapping any or all of {ILogger, Meter, ActivitySource }
partial class MyHomeControllerInstrumentation
{
    [Counter(Name="MyStrongTypedInstrument")]
    public partial void AddCounter(long increment, MyObject obj);
    [LoggerMessage(Message="Doing the work {clientName}")]
    public partial void LogDoWork(string clientName);
} 

class MyHomeController
{
    MyHomeControllerInstrumentation _instr;

    public MyHomeController(MyHomeControllerInstrumentation instrumentation)
    {
        _instr = instrumentation;
    }

    public void DoSomeWork(MyObject obj)
    {
        _instr.LogDoWork(obj.ClientName);
        _instr.AddCounter(10, obj);
        ...
    }
}

[dpk83]

@noahfalk When we were designing the solution we did consider something down that path, however, the creation of the instrument object becomes really challenging there.

As you didn't add where the creation of the actual counter is taking place based on the overall usage pattern I assume one of the following options.

1. Developers write the constructor for MyHomeControllerInstrumentation which takes in the instrument objects to be used for various instruments. So, if the class has Add methods for 10 different instruments, it will require the caller to pass 10 instrument objects when creating the MyHomeControllerInstrumentation object and then there is this challenge of how do you figure which instrument belongs to which of the Add* method.
2. MyHomeControllerInstrumentation constructor is also source generated. This will require a new attribute to be defined to be used on the class so a constructor can be generated. The problem of how do you map the Add* method with appropriate counter is still a problem here. Specially because now you have to make two different attributes (class attribute and the method attribute) work with each other during source generation.

[noahfalk]

I'm thinking of option 2 in your list above. I'm certainly inexperienced at implementing source generators so it is entirely possible I am overlooking obstacles, but so far in my experimentation the obstacles weren't apparent yet.

This will require a new attribute to be defined to be used on the class so a constructor can be generated.

I'm not sure why this would be the case? Hasn't the class already been identified by the presence of well-known attributes on its methods? I wrote a crude generator that found all methods decorated with the CounterAttribute, determined what class they were contained in and then generated a constructor for that class without the need for a 2nd attribute.

The problem of how do you map the Add* method with appropriate counter is still a problem here

I understand the mapping problem when you talked about option 1, but it was unclear how that problem applied in option 2. This is the user and generated code I was imagining:

// user code
partial class MyHomeControllerMetrics
{
    [Counter(Name="MyStrongTypedInstrument")]
    public partial void AddCounter(long increment, MyObject obj);
    [Counter(Name="TestInstrument")]
    public partial void AddTest(int increment, string clientRing, string clientType, string region);
} 

// generated code
partial class MyHomeControllerMetrics
{
    Counter<long> _c1;
    Counter<int> _c2;

    public MyHomeControllerMetrics(Meter m)
    {
        _c1 = m.CreateCounter<long>("MyStrongTypedInstrument");
        _c2 = m.CreateCounter<int>("TestInstrument");
    }

    public partial void AddCounter(long increment, MyObject obj)
    {
        //hand-waving the conversion of MyObject to tags because it is presumably the same
        //as whatever R9 is doing now in the strong-typed instrument code
        var tags = ...
        _c1.Add(increment, tags);
    }

    public partial void AddTest(int increment, string clientRing, string clientType, string region)
    {
        _c2.Add(increment, 
            KeyValuePair.Create("clientRing", clientRing),
            KeyValuePair.Create("clientType", clientType),
            KeyValuePair.Create("region", region));    
    }
} 

Looking at the Logging generator for comparison the generator appears to understand both the class and list of attributed methods in that class when generating the source. I was assuming the algorithm to generate the source here is roughly:

• scan the ordered list of attributed methods to create an ordered list of needed instruments
• foreach instrument in the list, write field to cache it
• write the constructor inside of which, foreach instrument in the list write the initializer
• foreach attributed method implement its body. The i'th method impl should invoke the i'th instrument because both lists are in the same order.

Maybe I am overlooking major issues and I'm hoping you can help me understand what they are. Thanks!

[dpk83]

I'm not sure why this would be the case? Hasn't the class already been identified by the presence of well-known attributes on its methods?

Yes, the presence of an attribute on the partial method in the class will let you modify the class but things get complicated as we are talking about the class containing multiple attributes all trying to modifying the class. i.e. A class can have a mix of multiple counters and histograms. While each attribute only modifies it's method now it's also modifying the containing class constructor.

Multiple attributes can't coordinate during source generation so having multiple attributes trying to modifying their containing class constructor doesn't look right.

e.g. In your example above each attribute will now need to add a call to create the counter/histogram in the class constructor, so each attribute will then try to create the constructor. As constructors can't be partial you will now add additional logic to check for the presence of constructor and then create one or use an existing one, or you entirely decide that you want to just create the instrument objects directly at the time of declaration.

Another issue with this approach is the fact that all declared instruments are allocated regardless of whether there is any code path using the object. It's a minor point but I am just stating it to surface the limitations.

@geeknoid Let us know your thoughts on the proposal once you are back next week.

[noahfalk]

In your example above each attribute will now need to add a call to create the counter/histogram in the class constructor, so each attribute will then try to create the constructor

Sorry I'm still having trouble understanding where this restriction is coming from? When I look at the existing logger source generator the code path appears to generate the entire class in one operation and the code is aware of all attributed methods in the class simultaneously. Am I misunderstanding something about how the logger code works or is there a reason the counter generator can't work the same way that the logger generator does? Thanks for trying to explain and sorry if I am just missing something.

[dpk83]

My mistake. You are right, parser is the one where we process each attributes individually and create the collection of all processed attributes so Emitter has the full collection to help aid with source generation.

Your proposal in general look fine but I have a few concerns

• All instrument objects are allocated regardless of whether they are used or not. e.g. you might want to enable some counters only based on some configuration.
• Tag key names can't have characters which aren't allowed as parameter names i.e. you can't have a tag key name client.ring. (It is solvable but the solution won't look nice)
• Somehow it feels a little more complex from usage point i.e. As a developer I usually create the instrument once but may call Add/Record multiple times at multiple different places. At every call I now see N different Add*/Record* method in intellisense and I have to chose vs in earlier case the name is always consistent Add() for counter and Record() for histogram. You generally have 1 or a couple of counters per class so less things to think about. Anyways, the existing approach feels much easier as a user to me, but that's my opinion and different users might feel differently.

I am not opposed to your proposal. We had multiple iteration before we landed on what we have today in R9 but can't remember all the pros and cons of various approach now so trying to recall some of the issues.

[noahfalk]

Thanks @dpk83! If you are able to find any of your notes or customer feedback from earlier iterations that could help give us a jump start. If not no worries, its still very useful to raise the pros/cons we see on the different designs so we can make sure to get feedback around it.

As I was thinking on this more the last few days I realized I was overlooking the relative lifespans of the Meter and Instrument objects. The example usages I gave above suggested that Instruments were created once per controller, but Meter was created as a DI singleton and injected via constructor. Since controller instances are created new for every new web request this means Instruments would also be created for every request. Meter objects maintain an internal List of every Instrument they create so those examples I gave would have nasty memory leaks. Was that an issue you had looked into before? In the original design you have at the top of the thread is a new Instrument instance allocated every time CreateTestInstrument(Meter) is called or will it return a cached previous allocation?

[dpk83]

If you are able to find any of your notes or customer feedback from earlier iterations that could help give us a jump start. If not no worries, its still very useful to raise the pros/cons we see on the different designs so we can make sure to get feedback around it.

We never released the other iterations to customers, they were more of our internal iterations as we were designing the apis. Most of the discussions were over meetings and in Teams chats so I will look for the pros/cons and share.

In the original design you have at the top of the thread is a new Instrument instance allocated every time CreateTestInstrument(Meter) is called or will it return a cached previous allocation?

In our original design we returned cached instrument (where we were using R9's Meter APIs). We ported the source gen to the new .NET Meter API recently which is where we didn't port the caching logic as we thought your team may not want to cache it given the .net meter APIs always create a new Meter and counter while R9's meter APIs supported caching.

Currently we have both the source gen and our customers are currently on the R9 meter APIs so are using the cached version and seems like we will have to add caching to the .NET Meter source gen as well to support migration to .NET meter without breaking their existing usage.

[noahfalk]

Ah gotcha, that makes more sense now. If we are going to do instrument caching (which I am hesitant to do but its certainly on the table) I'm thinking it would be more efficient and consistent to do it inside Meter rather than in the source generator only. I'm guessing you'd be happy with that if we wound up going that route? Is this source the right place to be looking for R9's current caching behavior?

Going back to some of the issues you raised above let me offer a few thoughts to see if we can find closer agreement, but its also fine if we don't. I'm hoping once we can solicit wider feedback we'll start seeing some majority preferences.

All instrument objects are allocated regardless of whether they are used or not

I wasn't expecting we'd see use-cases where large numbers of instruments had been defined in code but are never used. Did you see anything like that from R9's existing customers? I feel like this might be anti-pattern regardless because if they are defining so many instruments that the memory usage makes a measurable difference (1000+?), that probably means all the unused code loaded in memory is also making a measurable difference. One option to avoid this is to create the instruments on first use rather than in the constructor, but in return all users pay a tiny perf penalty for the extra conditional check.

Tag key names can't have characters which aren't allowed as parameter names i.e. you can't have a tag key name client.ring. (It is solvable but the solution won't look nice)

This one I hadn't recognized, thanks for raising it! One potential solution would be code that looks like this:

[Counter(Name="TestInstrument")]
    public partial void AddTest(int increment, 
                                [Tag(Name="client.Ring")] string clientRing,
                                [Tag(Name="client.Type")] string clientType,
                                string region);

I agree it looks less nice than before though we may disagree on the amount less nice. To me this is only mildly less nice. Another alternative is to have the generated type expose the instrument like this:

    [Counter<int>("clientRing", "clientType", "region", Name = "TestInstrument")]
    public TestInstrument TestInstrument {get;init;}

I think of this as being somewhere in the middle of the existing proposals. The attribute is identical to the static factory approach, but instrument is still being cached locally in a field and the Meter does not need to provided at the callsite. When you do the static factory design do you assume that all tags are string typed objects or is there some way in the attribute to specify that certain tags have a non-string type?

At every call I now see N different Add*/Record* method in intellisense and I have to chose vs in earlier case the name is always consistent Add() for counter and Record() for histogram

This feels like we shifted where the choice from N elements is made, but both designs still require the dev to make that choice at some point. In the static factory case the developer will need to pick from one of the N different CreateXYZ() methods when typing MyHomeControllerMetrics.CreateTestInstrument(meter); in the constructor. At the point of use they will also need to pick one of N different local variables when typing _test.Add(...). I'm assuming that the usual pattern is to create local variables for most or all CreateXYZ() functions on the type so the number of locals will be a similar sized N. When I look at our logging source generator pattern, that one requires the user to select one of N different LogXYZ() functions each time and I don't recall hearing any negative feedback around it. Agreed the method won't be Add() or Record(). I'm not anticipating many devs to care about that part of the change in a similar way that I don't think many devs minded replacing a standardized logger.LogInfo(...) with more varied API names when using the logger source generator.

[dpk83]

Is this source the right place to be looking for current caching behavior?

Yes

I wasn't expecting we'd see use-cases where large numbers of instruments had been defined in code but are never used. Did you see anything like that from your existing customers? I feel like this might be anti-pattern regardless because if they are defining so many instruments that the memory usage makes a measurable difference (1000+?), that probably means all the unused code loaded in memory is also making a measurable difference. One option to avoid this is to create the instruments on first use rather than in the constructor, but in return all users pay a tiny perf penalty for the extra conditional check.

Agree it's a minor point and I don't expect services to be doing it in general.

[Counter("clientRing", "clientType", "region", Name = "TestInstrument")]
public TestInstrument TestInstrument {get;init;}
I think of this as being somewhere in the middle of the existing proposals. The attribute is identical to the static factory approach, but instrument is still being cached locally in a field and the Meter does not need to provided at the callsite.

This actually looks nice, the only concern is that user will need to create different class for each meter object, i.e. if there are 10 different meter objects then a user will need to create 10 different meter classes. On the other hand I don't expect services to be using a lot of different meter instances.

When you do the static factory design do you assume that all tags are string typed objects or is there some way in the attribute to specify that certain tags have a non-string type?

Yes, we currently assume string by default because the metric backends only work with string tags (our metering implementation used strings so did the source gen), but that can easily be changed to accept object? instead given .NET Meter API accepts <string, object?> key value pair.

[noahfalk]

if there are 10 different meter objects then a user will need to create 10 different meter classes.

With the static factory approach was it considered good or common practice to pass different Meters into different factory functions defined on the same type. To me this pattern seems like it could easily set the developer up to unintentionally pass the wrong Meter.

    // Make sure to only call these functions with the Foo Meter
    [Counter<long>(typeof(MyObject), Name="MyStrongTypedInstrument")
    public static partial MyCounterInstrument CreateMyCounterInstrument(this IMeter meter);
    ...

    // Make sure to only call these functions with the Bar Meter
    [Counter<int>("clientRing", "clientType", "region", Name = "TestInstrument")]
    public static partial TestInstrument CreateTestInstrument(this IMeter meter);
    ...

but that can easily be changed to accept object? instead given .NET Meter API accepts <string, object?> key value pair.

I was more thinking that since the design was producing strongly typed instruments it felt a little odd that it wasn't enforcing strong typing on the tags. For example clientType sounds like it might be represented by an enum in the code and strong typing could make it clear to the developer that they must specify an enum value when passing that argument. However I can also imagine that non-string tags may be uncommon and it makes the attributes more verbose if the developer is required to enumerate the types for all tags.

[dpk83]

With the static factory approach was it considered good or common practice to pass different Meters into different factory functions defined on the same type. To me this pattern seems like it could easily set the developer up to unintentionally pass the wrong Meter.

It is a problem but wasn't a big concern because we expected the instrument creation will take part once during initialization (mostly in constructors) so developers will pass the required meter object.

I was more thinking that since the design was producing strongly typed instruments it felt a little odd that it wasn't enforcing strong typing on the tags. For example clientType sounds like it might be represented by an enum in the code and strong typing could make it clear to the developer that they must specify an enum value when passing that argument. However I can also imagine that non-string tags may be uncommon and it makes the attributes more verbose if the developer is required to enumerate the types for all tags.

Specifying type for each tag would have compromised the usability as well as the fact that we have to ultimately convert it to string it would result in boxing which we definitely want to avoid as much as possible for performance reason here.

[tarekgh]

@dpk83 per offline discussions I have moved this to future milestone out of .NET 8.0.