[API Proposal]: System.Diagnostics.Activity: Enumeration API [Iteration 2]

[CodeBlanch]

First proposal: #67072

Background and motivation

One of the main things the OTel .NET SDK needs to do is export Activity (trace) data. There is a lot of enumeration needed to do that. Activity tags, links(+tags), and events(+tags). Today the .NET API is all IEnumerable<T>/IEnumerator<T> based and all of the items being enumerated are readonly structs. Our performance testing was showing a lot of allocations incurred using the enumeration interfaces. To avoid that we generate IL to bind to the internal struct contracts. That fixes the allocations but a lot time is currently being spent copying value types.

What would be great is if the .NET API exposed a public method for enumerating these elements without allocation and as readonly ref to avoid copying.

/cc @reyang @cijothomas

API Proposal

The idea is to expose a base class which users may use (with a cast) to enumerate Activity data more efficiently:

namespace System.Diagnostics
{
    // New
    public abstract class ActivityItemEnumerable<T> : System.Collections.Generic.IEnumerable<T>
    {
        public abstract Enumerator GetEnumerator();
        System.Collections.Generic.IEnumerator<T> System.Collections.Generic.IEnumerable<T>.GetEnumerator() { throw null; }
        System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() { throw null; }

        public struct Enumerator : System.Collections.Generic.IEnumerator<T>
        {
            public readonly ref T Current { get { throw null; } }
            T System.Collections.Generic.IEnumerator<T>.Current { get { throw null; } }
            object? System.Collections.IEnumerator.Current { get { throw null; } }
            public bool MoveNext() { throw null; }
            void System.Collections.IEnumerator.Reset() { }
            public void Dispose() { }
        }
    }
}

API Usage

        public int EnumerateTagsUsingNewApi()
        {
            ActivityItemEnumerable<KeyValuePair<string, object>> enumerable = (ActivityItemEnumerable<KeyValuePair<string, object>>)this.activity.TagObjects;

            int count = 0;
            foreach (ref readonly KeyValuePair<string, object> tag in enumerable)
            {
                if (tag.Value != null)
                {
                    count++;
                }
            }

            return count;
        }

Example implementation

Diff: https://github.com/dotnet/runtime/compare/main...CodeBlanch:activity-enumerator?expand=1

Alternatives / considerations

Originally I wanted to change the return types on the Activity class but chatting with @tarekgh that would be a breaking change. Decided the cast would work well enough.

Not part of the proposal, but desired:

namespace System.Diagnostics
{
    public abstract class Activity
    {
-        public System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, object?>> TagObjects { get { throw null; } }
-        public System.Collections.Generic.IEnumerable<System.Diagnostics.ActivityEvent> Events { get { throw null; } }
-        public System.Collections.Generic.IEnumerable<System.Diagnostics.ActivityLink> Links { get { throw null; } }
+        public System.Diagnostics.DiagnosticEnumerable<System.Collections.Generic.KeyValuePair<string, object?>> TagObjects { get { throw null; } }
+        public System.Diagnostics.DiagnosticEnumerable<System.Diagnostics.ActivityEvent> Events { get { throw null; } }
+        public System.Diagnostics.DiagnosticEnumerable<System.Diagnostics.ActivityLink> Links { get { throw null; } }
   }
}

Instead of forcing a cast we could modify the Activity API to expose a strongly typed API at the cost of some duplication/confusion:

namespace System.Diagnostics
{
    public abstract class Activity
    {
        public System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, object?>> TagObjects { get { throw null; } }
        public System.Collections.Generic.IEnumerable<System.Diagnostics.ActivityEvent> Events { get { throw null; } }
        public System.Collections.Generic.IEnumerable<System.Diagnostics.ActivityLink> Links { get { throw null; } }
+       public System.Diagnostics.DiagnosticEnumerable<System.Collections.Generic.KeyValuePair<string, object?>> TagObjectsEnumerable { get { throw null; } }
+       public System.Diagnostics.DiagnosticEnumerable<System.Diagnostics.ActivityEvent> EventsEnumerable { get { throw null; } }
+       public System.Diagnostics.DiagnosticEnumerable<System.Diagnostics.ActivityLink> LinksEnumerable { get { throw null; } }
   }
}

Testing + benchmarks

.NET 6 API w/ OTel SDK

Method	Mean	Error	StdDev	Gen 0	Allocated
EnumerateTags	129.53 ns	2.354 ns	2.087 ns	0.0095	40 B
EnumerateTagsUsingReflection	37.28 ns	0.280 ns	0.248 ns	-	-

.NET 7 API w/ OTel SDK

Note: .NET 7 build contains already merged #67012 so the base cases are seeing some improvement from that optimization.

Method	Mean	Error	StdDev	Gen 0	Allocated
EnumerateTags	94.96 ns	1.927 ns	3.892 ns	0.0076	32 B
EnumerateTagsUsingReflection*	20.21 ns	0.423 ns	0.503 ns	-	-
EnumerateTagsUsingNewApi	15.82 ns	0.250 ns	0.234 ns	-	-

* The proposed API implementation does not break the existing OTel SDK reflection/IL generation.

Benchmark code

using System;
using System.Collections.Generic;
using System.Diagnostics;
using BenchmarkDotNet.Attributes;
using OpenTelemetry.Trace;

namespace Benchmarks.Trace
{
    [MemoryDiagnoser]
    public class ActivityEnumerationBenchmarks : IDisposable
    {
        private readonly ActivitySource source = new("Source");
        private readonly Activity activity;

        public ActivityEnumerationBenchmarks()
        {
            Activity.DefaultIdFormat = ActivityIdFormat.W3C;

            ActivitySource.AddActivityListener(new ActivityListener
            {
                ActivityStarted = null,
                ActivityStopped = null,
                ShouldListenTo = (activitySource) => activitySource.Name == "Source",
                Sample = (ref ActivityCreationOptions<ActivityContext> options) => ActivitySamplingResult.AllDataAndRecorded,
            });

            this.activity = this.source.StartActivity("test");

            this.activity.SetTag("tag1", "value1");
            this.activity.SetTag("tag2", "value2");
            this.activity.SetTag("tag3", "value3");
            this.activity.SetTag("tag4", "value4");
            this.activity.SetTag("tag5", "value5");

            this.activity.Stop();
        }

        public void Dispose()
        {
            this.source.Dispose();
        }

        [Benchmark]
        public int EnumerateTags()
        {
            int count = 0;
            foreach (KeyValuePair<string, object> tag in this.activity.TagObjects)
            {
                if (tag.Value != null)
                {
                    count++;
                }
            }

            return count;
        }

        [Benchmark]
        public int EnumerateTagsUsingReflection()
        {
            TagEnumerationState state = default;

            this.activity.EnumerateTags(ref state);

            return state.Count;
        }

        [Benchmark]
        public int EnumerateTagsUsingNewApi()
        {
            ActivityItemEnumerable<KeyValuePair<string, object>> enumerable = (ActivityItemEnumerable<KeyValuePair<string, object>>)this.activity.TagObjects;

            int count = 0;
            foreach (ref readonly KeyValuePair<string, object> tag in enumerable)
            {
                if (tag.Value != null)
                {
                    count++;
                }
            }

            return count;
        }

        private struct TagEnumerationState : IActivityEnumerator<KeyValuePair<string, object>>
        {
            public int Count;

            public bool ForEach(KeyValuePair<string, object> activityTag)
            {
                if (activityTag.Value != null)
                {
                    this.Count++;
                }

                return true;
            }
        }
    }
}

[ghost]

Tagging subscribers to this area: @dotnet/area-system-diagnostics-activity
See info in area-owners.md if you want to be subscribed.

Issue Details

---

First proposal: #67072

Background and motivation

One of the main things the OTel .NET SDK needs to do is export Activity (trace) data. There is a lot of enumeration needed to do that. Activity tags, links(+tags), and events(+tags). Today the .NET API is all IEnumerable<T>/IEnumerator<T> based and all of the items being enumerated are readonly structs. Our performance testing was showing a lot of allocations incurred using the enumeration interfaces. To avoid that we generate IL to bind to the internal struct contracts. That fixes the allocations but a lot time is currently being spent copying value types.

What would be great is if the .NET API exposed a public method for enumerating these elements without allocation and as readonly ref to avoid copying.

/cc @reyang @cijothomas

API Proposal

The idea is to expose a base class which users may use (with a cast) to enumerate Activity data more efficiently:

namespace System.Diagnostics
{
    // New
    public abstract class DiagnosticEnumerable<T> : System.Collections.Generic.IEnumerable<T>
    {
        public abstract Enumerator GetEnumerator();
        System.Collections.Generic.IEnumerator<T> System.Collections.Generic.IEnumerable<T>.GetEnumerator() { throw null; }
        System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() { throw null; }

        public struct Enumerator : System.Collections.Generic.IEnumerator<T>
        {
            public readonly ref T Current { get { throw null; } }
            T System.Collections.Generic.IEnumerator<T>.Current { get { throw null; } }
            object? System.Collections.IEnumerator.Current { get { throw null; } }
            public bool MoveNext() { throw null; }
            public void Reset() { }
            public void Dispose() { }
        }
    }
}

API Usage

        public int EnumerateTagsUsingNewApi()
        {
            DiagnosticEnumerable<KeyValuePair<string, object>> enumerable = (DiagnosticEnumerable<KeyValuePair<string, object>>)this.activity.TagObjects;

            int count = 0;
            foreach (ref readonly KeyValuePair<string, object> tag in enumerable)
            {
                if (tag.Value != null)
                {
                    count++;
                }
            }

            return count;
        }

Example implementation

Diff: https://github.com/dotnet/runtime/compare/main...CodeBlanch:activity-enumerator?expand=1

Alternatives / considerations

Originally I wanted to change the return types on the Activity class but chatting with @tarekgh that would be a breaking change. Decided the cast would work well enough.

Not part of the proposal, but desired:

namespace System.Diagnostics
{
    public abstract class Activity
    {
-        public System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, object?>> TagObjects { get { throw null; } }
-        public System.Collections.Generic.IEnumerable<System.Diagnostics.ActivityEvent> Events { get { throw null; } }
-        public System.Collections.Generic.IEnumerable<System.Diagnostics.ActivityLink> Links { get { throw null; } }
+        public System.Diagnostics.DiagnosticEnumerable<System.Collections.Generic.KeyValuePair<string, object?>> TagObjects { get { throw null; } }
+        public System.Diagnostics.DiagnosticEnumerable<System.Diagnostics.ActivityEvent> Events { get { throw null; } }
+        public System.Diagnostics.DiagnosticEnumerable<System.Diagnostics.ActivityLink> Links { get { throw null; } }
   }
}

Instead of forcing a cast we could modify the Activity API to expose a strongly typed API at the cost of some duplication/confusion:

namespace System.Diagnostics
{
    public abstract class Activity
    {
        public System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, object?>> TagObjects { get { throw null; } }
        public System.Collections.Generic.IEnumerable<System.Diagnostics.ActivityEvent> Events { get { throw null; } }
        public System.Collections.Generic.IEnumerable<System.Diagnostics.ActivityLink> Links { get { throw null; } }
+       public System.Diagnostics.DiagnosticEnumerable<System.Collections.Generic.KeyValuePair<string, object?>> TagObjectsEnumerable { get { throw null; } }
+       public System.Diagnostics.DiagnosticEnumerable<System.Diagnostics.ActivityEvent> EventsEnumerable { get { throw null; } }
+       public System.Diagnostics.DiagnosticEnumerable<System.Diagnostics.ActivityLink> LinksEnumerable { get { throw null; } }
   }
}

Testing + benchmarks

.NET 6 API w/ OTel SDK

Method	Mean	Error	StdDev	Gen 0	Allocated
EnumerateTags	129.53 ns	2.354 ns	2.087 ns	0.0095	40 B
EnumerateTagsUsingReflection	37.28 ns	0.280 ns	0.248 ns	-	-

.NET 7 API w/ OTel SDK

Note: .NET 7 build contains already merged #67012 so the base cases are seeing some improvement from that optimization.

Method	Mean	Error	StdDev	Gen 0	Allocated
EnumerateTags	94.96 ns	1.927 ns	3.892 ns	0.0076	32 B
EnumerateTagsUsingReflection*	20.21 ns	0.423 ns	0.503 ns	-	-
EnumerateTagsUsingNewApi	15.82 ns	0.250 ns	0.234 ns	-	-

* The proposed API implementation does not break the existing OTel SDK reflection/IL generation.

Benchmark code

using System;
using System.Collections.Generic;
using System.Diagnostics;
using BenchmarkDotNet.Attributes;
using OpenTelemetry.Trace;

namespace Benchmarks.Trace
{
    [MemoryDiagnoser]
    public class ActivityEnumerationBenchmarks : IDisposable
    {
        private readonly ActivitySource source = new("Source");
        private readonly Activity activity;

        public ActivityEnumerationBenchmarks()
        {
            Activity.DefaultIdFormat = ActivityIdFormat.W3C;

            ActivitySource.AddActivityListener(new ActivityListener
            {
                ActivityStarted = null,
                ActivityStopped = null,
                ShouldListenTo = (activitySource) => activitySource.Name == "Source",
                Sample = (ref ActivityCreationOptions<ActivityContext> options) => ActivitySamplingResult.AllDataAndRecorded,
            });

            this.activity = this.source.StartActivity("test");

            this.activity.SetTag("tag1", "value1");
            this.activity.SetTag("tag2", "value2");
            this.activity.SetTag("tag3", "value3");
            this.activity.SetTag("tag4", "value4");
            this.activity.SetTag("tag5", "value5");

            this.activity.Stop();
        }

        public void Dispose()
        {
            this.source.Dispose();
        }

        [Benchmark]
        public int EnumerateTags()
        {
            int count = 0;
            foreach (KeyValuePair<string, object> tag in this.activity.TagObjects)
            {
                if (tag.Value != null)
                {
                    count++;
                }
            }

            return count;
        }

        [Benchmark]
        public int EnumerateTagsUsingReflection()
        {
            TagEnumerationState state = default;

            this.activity.EnumerateTags(ref state);

            return state.Count;
        }

        [Benchmark]
        public int EnumerateTagsUsingNewApi()
        {
            DiagnosticEnumerable<KeyValuePair<string, object>> enumerable = (DiagnosticEnumerable<KeyValuePair<string, object>>)this.activity.TagObjects;

            int count = 0;
            foreach (ref readonly KeyValuePair<string, object> tag in enumerable)
            {
                if (tag.Value != null)
                {
                    count++;
                }
            }

            return count;
        }

        private struct TagEnumerationState : IActivityEnumerator<KeyValuePair<string, object>>
        {
            public int Count;

            public bool ForEach(KeyValuePair<string, object> activityTag)
            {
                if (activityTag.Value != null)
                {
                    this.Count++;
                }

                return true;
            }
        }
    }
}

Author:	CodeBlanch
Assignees:	tarekgh
Labels:	area-System.Diagnostics.Activity
Milestone:	-

[tarekgh]

@noahfalk @reyang @cijothomas this proposal looks good to me. any feedback?

[reyang]

LGTM 👍

[KalleOlaviNiemitalo]

If DiagnosticEnumerable<T>.Enumerator.Reset() is going to throw NotSupportedException, it should preferably be an explicit interface implementation.

[cijothomas]

        DiagnosticEnumerable<KeyValuePair<string, object>> enumerable = (DiagnosticEnumerable<KeyValuePair<string, object>>)this.activity.TagObjects;

^ It this part of the contract of Activity class, that TagObjects can be casted to the DiagnosticEnumerable?

[CodeBlanch]

@cijothomas It is more of an implementation detail, but critical to making this work! It does worry me that someone in the future could come along and not understand that and break things. If you check out the "Alternatives / considerations" section in the description, we could make the contract strongly typed by introducing some new properties. It would be nice to change the existing ones, but that would be breaking sadly 💔

I thought about doing something like this...

+public interface ITracingDataContainer
+{
+        public DiagnosticEnumerable<KeyValuePair<string, object?>> TagObjects { get { throw null; } }
+        public DiagnosticEnumerable<ActivityEvent> Events { get { throw null; } }
+        public DiagnosticEnumerable<ActivityLink> Links { get { throw null; } }
+}

public class Activity
+   : ITracingDataContainer
{
+        DiagnosticEnumerable<KeyValuePair<string, object?>> ITracingDataContainer.TagObjects { get { throw null; } }
+        DiagnosticEnumerable<ActivityEvent> ITracingDataContainer.Events { get { throw null; } }
+        DiagnosticEnumerable<ActivityLink> ITracingDataContainer.Links { get { throw null; } }
}

So we have an interface on Activity which is hidden/explicitly implemented to not confuse users with multiple ways of doing things, but enables the fast enumeration scenarios. Usage would be something like this:

        public int EnumerateTagsUsingNewApi()
        {
            ITracingDataContainer tracingDataContainer = this.activity;

            int count = 0;
            foreach (ref readonly KeyValuePair<string, object> tag in tracingDataContainer.TagObjects)
            {
                if (tag.Value != null)
                {
                    count++;
                }
            }

            return count;
        }

• Con: An additional public type (ITracingDataContainer).
• Pro: Everything becomes strongly-typed.

[tarekgh]

It does worry me that someone in the future could come along and not understand that and break things.

We should be adding tests to ensure no breaking will happen in the future. I am not seeing this will be a problem if we add such tests. Also, the consuming code (e.g., OpenTelemetry) can be written in a safe way too.

[noahfalk]

1. I liked @KalleOlaviNiemitalo's suggestion to leave NotSupported APIs as explicit interfaces only (as long as we confirm it doesn't interfere with C# compiler's ability to use the strongly-typed struct in a foreach statement without needing to box it)
2. DiagnosticEnumerable feels like an overly broad name in the System.Diagnostics namespace. What about narrowing the scope like this?

public class Activity
{
    public class Enumerable<T>
    {
    }
    public class Enumerator<T>
    {
    }
}

or for a non-nested option ActivityDataEnumerable<T>

3. For the strongly typed enumeration API via additional interface on Activity, I'm fine having it or skipping it for now and retaining the flexibility to add it later as needed. I defer to Tarek's choice.

[tarekgh]

For the strongly typed enumeration API via additional interface on Activity, I'm fine having it or skipping it for now and retaining the flexibility to add it later as needed. I defer to Tarek's choice.

I prefer not to have it at least for now to avoid increasing the confusion for the Activity users and it is not worth it to introduce a new Type just to support that.

[tarekgh]

@noahfalk suggestion is a good idea to have the new classes defined inside the Activity class.

@CodeBlanch if you agree, could you please update the proposal accordingly?

[CodeBlanch]

@tarekgh I don't have an issue with it, but one thing I wanted to bring up. If we do...

public class Activity
{
    public class Enumerable<T> {}
    public class Enumerator<T> {}
}

Then DiagLinkedList will need to be...

internal sealed class DiagLinkedList<T> : Activity.Enumerable<T>
{
   public override Activity.Enumerator<T> GetEnumerator() => new Activity.Enumerator<T>(_first);
}

So a type outside of Activity is using nested classes from Activity. That is all internal, so probably not a big deal, but thought I would run it by you to make sure.

[tarekgh]

@CodeBlanch

Thinking more about that, I am inclining to go back to the original proposal having the type DiagnosticEnumerable outside the Activity class. The reason is callers will need to cast to that type. Writing DiagnosticEnumerable<T> will be better than Activity.Enumerable<T> . Other reason is, in the future we may decide to expose more APIs using this type outside the Activity class.

@noahfalk what you think?

[noahfalk]

My main qualm about "DiagnosticEnumerable" is that the name is very non-specific. I think of this as a specific optimization for a narrow use-case rather than a general purpose exchange type that would have wide usage. For example in other cases we might want the enumerable to be backed by an array, a list, a segmented list, etc. A couple on thoughts on where we could go...

1. If we want to think of this as a general purpose faster API for enumeration that could be used in many contexts other than Activity then perhaps it should be in System.Collections.Generic or System.Collections.Specialized. Not sure if other folks on the BCL team would feel it has high enough utility to merit that and I'm skeptical, but we could certainly ask.

2. If we want the possibility to opt other use-cases into this implementation in the future while still keeping it tightly scoped to a small number of scenarios perhaps we should decouple the public API from the implementation doing something like this:

internal class DiagLinkedList { ... }

public struct ActivityItemEnumerable<T> : IEnumerable<T>
{
    DiagLinkedList _linkedList;
    public void ActivityItemEnumerator GetEnumerator() => _linkedList.GetEnumerator();
    // forward other APIs as needed
}

// we find a second scenario that also wants to enumerate the DiagLinkedList implementation, but we want to give
// it a name unrelated to Activity and reserve the right to change its implementation independent of Activity
public struct SomeOtherEnumerable<T> : IEnumerable<T>
{
    DiagLinkedList _linkedList;
    // forward APIs as needed
}

[tarekgh]

If we want to think of this as a general purpose faster API for enumeration that could be used in many contexts other than Activity then perhaps it should be in System.Collections.Generic or System.Collections.Specialized.

I was not thinking of using it that broad. I was thinking it can be used in the Activity/Metrics scenarios. I mean is it scoped to System.Diaganostics.DiagnosticSource library scenarios.

If we want the possibility to opt other use-cases into this implementation in the future while still keeping it tightly scoped to a small number of scenarios perhaps we should decouple the public API from the implementation doing something like this:

That is what I meant. But will your proposal here require exposing a new type (like SomeOtherEnumerable<T>) every time we encounter a new scenario outside Activity? If so, then this is the part I am not a fan of. I am not seeing exposing the same type twice with two different names would be perfect :-)

Anyway, as I am not sure if we'll use this type in other scenarios outside Activity, I can live with ActivityItemEnumerable name for now.

[noahfalk]

I am not seeing exposing the same type twice with two different names would be perfect :-)

I think having (potentially) multiple types is the cost we pay for keeping the implementation abstracted from users. These are the options I can find so far:

1. If we don't care about abstracting implementation then we could name this type LinkedListEnumerable<T> and expose it in all scenarios that use the DiagLinkedList. We could also name it DiagnosticEnumerable which doesn't make the relationship apparent in the name, but it would still be the case that scenarios can use DiagnosticEnumerable if and only if the underlying storage is DiagLinkedList. Even though that name would make it sound general purpose, it wouldn't be.

2. If we want to trade away some of the perf we could keep the implementation abstracted with an interface that is reusable regardless of backing storage:

public interface IByRefEnumerable<T>
{
    IByRefEnumerator<T> GetEnumerator();
}
public interface IByRefEnumerator<T>
{
    ref T Current { get; }
    bool MoveNext();
}

The downside of the interfaces is that we have to allocate when calling GetEnumerator() to return back a reference type.

3. If we want to keep the implementation abstracted and avoid the allocation for GetEnumerator() then we have to use a concrete type like ActivityItemEnumerable as the API surface rather than an interface. Having a few different types like ActivityEnumerable, MeterEnumerable, etc means we aren't promising that all the scenarios have matching storage implementations as part of the API, but we still have the option to share behind the scenes.

Option 3 seems the best to me, but if there is another option I am missing or you guys think one of the other options is a better tradeoff I'm open to it. I don't think there is any option which is clearly the best across all criteria we care about.

[KalleOlaviNiemitalo]

If you ever change some properties of Activity to use a backing type other than DiagLinkedList, you can in principle do that without breaking uses of struct DiagnosticEnumerator, by adding runtime checks for supporting a small set of other backing types. And introduce the more specific enumerable/enumerator types in that version so that users have a way to avoid the runtime checks.

[CodeBlanch]

My vote would be for 1 or 3 from above. For 3, ActivityItemEnumerable<T> or ActivityDataEnumerable<T> both seem reasonable to me. Not passionate about DiagnosticEnumerable<T> I just threw that out to get the conversation started.

[tarekgh]

Ok, let's go with ActivityItemEnumerable<T> then. @CodeBlanch could you please modify the proposal one last time? Thanks!

[CodeBlanch]

Updated

[bartonjs]

Video

Rather than expose an abstract class to describe the concept, we feel that it's solvable with just one struct iterator type defined on Activity

namespace System.Diagnostics
{
    partial class Activity
    {
        public Enumerator<KeyValuePair<string,object>> EnumerateTagObjects();
        public Enumerator<ActivityLink> EnumerateLinks();
        public Enumerator<ActivityEvent> EnumerateEvents();
    
        public struct Enumerator<T>
        {
            private Enumerator(/* The node */)  { }
            public readonly Enumerator<T> GetEnumerator() => this;

            public readonly ref T Current => ref _node.Value;
            public bool MoveNext() { ... }
        }
    }
}

[CodeBlanch]

@tarekgh I updated the implementation branch I have been using for the new API surface. One hiccup, in order to return the new Activity.Enumerator<T> via the existing IEnumerable<T> props it needs to declare/implement the IEnumerator<T> contract. We could keep the existing internal enumerator to drive those (and have 2) but I'm guessing it isn't a big deal to add that to the design?

[CodeBlanch]

One of the questions brought up on the design review was (more or less): Does this lead to a measurable impact on the export process inside the OpenTelemetry SDK?

Here are some benchmarks to try and answer that question.

Scenarios:

1. Single activity with 6 tags.
2. Single activity with 6 tags + single event with 3 tags. This would be the case of an exception, which is added as an event with tags for the data.
3. Single activity with 6 tags + 3 events each with 3 tags. This would be the case of users attaching logs as events to the current trace.

Exporter: Jaeger, which writes its data to a buffer that is reused across all export calls.

I tried to pick somewhat realistic cases. No cases included for links because I don't think those are used much today. Even though it is the biggest struct (?) and might benefit the most from being passed around as ref.

Using .NET 6 IEnumerable APIs:

Method	NumberOfEvents	Mean	Error	StdDev	Gen 0	Allocated
JaegerExporter_Batching	0	2.326 us	0.0027 us	0.0025 us	0.0076	104 B
JaegerExporter_Batching	1	3.262 us	0.0025 us	0.0024 us	0.0153	208 B
JaegerExporter_Batching	3	4.933 us	0.0042 us	0.0037 us	0.0229	304 B

Using .NET 6 IEnumerable API with reflection engine:

Method	NumberOfEvents	Mean	Error	StdDev	Allocated
JaegerExporter_Batching	0	2.216 us	0.0040 us	0.0038 us	-
JaegerExporter_Batching	1	3.090 us	0.0049 us	0.0046 us	-
JaegerExporter_Batching	3	4.690 us	0.0030 us	0.0024 us	-

Using local build of .NET 7 with proposed API*:

Method	NumberOfEvents	Mean	Error	StdDev	Allocated
JaegerExporter_Batching	0	2.151 us	0.0021 us	0.0019 us	-
JaegerExporter_Batching	1	3.002 us	0.0020 us	0.0018 us	-
JaegerExporter_Batching	3	4.636 us	0.0054 us	0.0048 us	-

*Still using the reflection engine to enumerate ActivityEvent.Tags because that API is not part of this proposal. If this gets approved/merged we can work on that as well.