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

[CodeBlanch]

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

namespace System.Diagnostics
{
    public class Activity
    {
          public ReadOnlySpan<KeyValuePair<string, object?>> TagsAsReadOnlySpan();
          public ReadOnlySpan<ActivityLink> LinksAsReadOnlySpan();
          public ReadOnlySpan<ActivityEvent> EventsAsReadOnlySpan();
    }

    public class ActivityTagsCollection
    {
        public ReadOnlySpan<KeyValuePair<string, object?>> AsReadOnlySpan();
    }
}

API Usage

In some cases we export to DTOs. Other cases we export to some reusable buffer. Worth noting, OTel Export API is not async. Export either runs inline on calling thread ("simple mode") or on a dedicated background thread ("batch mode").

public static Dto ToDto(this Activity activity)
{
   var dto = new Dto();

   foreach (ref readonly KeyValuePair<string, object?> tag in activity.TagsAsReadOnlySpan())
   {
       dto.Attributes.Add(CreateDtoTag(in tag));
   }

   foreach (ref readonly ActivityLink link in activity.LinksAsReadOnlySpan())
   {
       dto.Links.Add(CreateDtoLink(in link));
   }

   foreach (ref readonly ActivityEvent @event in activity.EventsAsReadOnlySpan())
   {
       dto.Events.Add(CreateDtoEvent(in @event));
   }

   return dto;
}

private static DtoLink CreateDtoLink(in ActivityLink link)
{
   vat dto = new DtoLink();

   ActivityTagsCollection tags = (ActivityTagsCollection)link.Tags; // Currently IEnumerable<KeyValuePair<string, object?>> so needs a cast. Would be nice to make it concrete type but not strictly required.

   foreach (ref readonly KeyValuePair<string, object?> tag in tags.TagsAsReadOnlySpan())
   {
       dto.Attributes.Add(CreateDtoTag(in tag));
   }

   return dto;
}

// Not shown CreateDtoTag or CreateDtoEvent but they will work just like the above.

Alternative Designs

I did some performance testing. Using ReadOnlySpan<T> had the best perf. ActivityTagsCollection is List<T> based, so might not be a challenge to expose ROS there. Activity versions are using a custom internal linked list. Might be difficult to expose ROS there without changing the underlying data structure. I thought I would shoot for the moon first so I asked for the ROS API but an alternative API would be to expose more traditional structs for enumeration that have a readonly ref T Current. For example:

    public struct Enumerator<T> : IEnumerator<T>
    {
        private static readonly DiagNode<T> s_Empty = new DiagNode<T>(default!);

        private DiagNode<T>? _nextNode;
        private DiagNode<T> _currentNode;

        public Enumerator(DiagNode<T>? head)
        {
            _nextNode = head;
            _currentNode = s_Empty;
        }

        public readonly ref T Current => ref _currentNode.Value;

        T IEnumerator<T>.Current => Current;

        object? IEnumerator.Current => Current;

        public bool MoveNext()
        {
            if (_nextNode == null)
            {
                _currentNode =  s_Empty;
                return false;
            }

            _currentNode = _nextNode;
            _nextNode = _nextNode.Next;
            return true;
        }

        public void Reset() => throw new NotSupportedException();

        public void Dispose()
        {
        }
    }

If the ROS option is not viable I will spend more time flushing this out.

Risks

None that I can think of.

[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.

[ghost]

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

Issue Details

---

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

namespace System.Diagnostics
{
    public class Activity
    {
          public ReadOnlySpan<KeyValuePair<string, object?>> TagsAsReadOnlySpan();
          public ReadOnlySpan<ActivityLink> LinksAsReadOnlySpan();
          public ReadOnlySpan<ActivityEvent> EventsAsReadOnlySpan();
    }

    public class ActivityTagsCollection
    {
        public ReadOnlySpan<KeyValuePair<string, object?>> AsReadOnlySpan();
    }
}

API Usage

In some cases we export to DTOs. Other cases we export to some reusable buffer. Worth noting, OTel Export API is not async. Export either runs inline on calling thread ("simple mode") or on a dedicated background thread ("batch mode").

public static Dto ToDto(this Activity activity)
{
   var dto = new Dto();

   foreach (ref readonly KeyValuePair<string, object?> tag in activity.TagsAsReadOnlySpan())
   {
       dto.Attributes.Add(CreateDtoTag(in tag));
   }

   foreach (ref readonly ActivityLink link in activity.LinksAsReadOnlySpan())
   {
       dto.Links.Add(CreateDtoLink(in link));
   }

   foreach (ref readonly ActivityEvent @event in activity.EventsAsReadOnlySpan())
   {
       dto.Events.Add(CreateDtoEvent(in @event));
   }

   return dto;
}

private static DtoLink CreateDtoLink(in ActivityLink link)
{
   vat dto = new DtoLink();

   ActivityTagsCollection tags = (ActivityTagsCollection)link.Tags; // Currently IEnumerable<KeyValuePair<string, object?>> so needs a cast. Would be nice to make it concrete type but not strictly required.

   foreach (ref readonly KeyValuePair<string, object?> tag in tags.TagsAsReadOnlySpan())
   {
       dto.Attributes.Add(CreateDtoTag(in tag));
   }

   return dto;
}

// Not shown CreateDtoTag or CreateDtoEvent but they will work just like the above.

Alternative Designs

I did some performance testing. Using ReadOnlySpan<T> had the best perf. ActivityTagsCollection is List<T> based, so might not be a challenge to expose ROS there. Activity versions are using a custom internal linked list. Might be difficult to expose ROS there without changing the underlying data structure. I thought I would shoot for the moon first so I asked for the ROS API but an alternative API would be to expose more traditional structs for enumeration that have a readonly ref T Current. For example:

    public struct Enumerator<T> : IEnumerator<T>
    {
        private static readonly DiagNode<T> s_Empty = new DiagNode<T>(default!);

        private DiagNode<T>? _nextNode;
        private DiagNode<T> _currentNode;

        public Enumerator(DiagNode<T>? head)
        {
            _nextNode = head;
            _currentNode = s_Empty;
        }

        public readonly ref T Current => ref _currentNode.Value;

        T IEnumerator<T>.Current => Current;

        object? IEnumerator.Current => Current;

        public bool MoveNext()
        {
            if (_nextNode == null)
            {
                _currentNode =  s_Empty;
                return false;
            }

            _currentNode = _nextNode;
            _nextNode = _nextNode.Next;
            return true;
        }

        public void Reset() => throw new NotSupportedException();

        public void Dispose()
        {
        }
    }

If the ROS option is not viable I will spend more time flushing this out.

Risks

None that I can think of.

Author:	CodeBlanch
Assignees:	-
Labels:	api-suggestion, area-System.Diagnostics.Tracing, untriaged
Milestone:	-

[tarekgh]

@CodeBlanch how you think XXXAsReadOnlySpan will get implemented to help with the perf here?

[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

---

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

namespace System.Diagnostics
{
    public class Activity
    {
          public ReadOnlySpan<KeyValuePair<string, object?>> TagsAsReadOnlySpan();
          public ReadOnlySpan<ActivityLink> LinksAsReadOnlySpan();
          public ReadOnlySpan<ActivityEvent> EventsAsReadOnlySpan();
    }

    public class ActivityTagsCollection
    {
        public ReadOnlySpan<KeyValuePair<string, object?>> AsReadOnlySpan();
    }
}

API Usage

In some cases we export to DTOs. Other cases we export to some reusable buffer. Worth noting, OTel Export API is not async. Export either runs inline on calling thread ("simple mode") or on a dedicated background thread ("batch mode").

public static Dto ToDto(this Activity activity)
{
   var dto = new Dto();

   foreach (ref readonly KeyValuePair<string, object?> tag in activity.TagsAsReadOnlySpan())
   {
       dto.Attributes.Add(CreateDtoTag(in tag));
   }

   foreach (ref readonly ActivityLink link in activity.LinksAsReadOnlySpan())
   {
       dto.Links.Add(CreateDtoLink(in link));
   }

   foreach (ref readonly ActivityEvent @event in activity.EventsAsReadOnlySpan())
   {
       dto.Events.Add(CreateDtoEvent(in @event));
   }

   return dto;
}

private static DtoLink CreateDtoLink(in ActivityLink link)
{
   vat dto = new DtoLink();

   ActivityTagsCollection tags = (ActivityTagsCollection)link.Tags; // Currently IEnumerable<KeyValuePair<string, object?>> so needs a cast. Would be nice to make it concrete type but not strictly required.

   foreach (ref readonly KeyValuePair<string, object?> tag in tags.TagsAsReadOnlySpan())
   {
       dto.Attributes.Add(CreateDtoTag(in tag));
   }

   return dto;
}

// Not shown CreateDtoTag or CreateDtoEvent but they will work just like the above.

Alternative Designs

I did some performance testing. Using ReadOnlySpan<T> had the best perf. ActivityTagsCollection is List<T> based, so might not be a challenge to expose ROS there. Activity versions are using a custom internal linked list. Might be difficult to expose ROS there without changing the underlying data structure. I thought I would shoot for the moon first so I asked for the ROS API but an alternative API would be to expose more traditional structs for enumeration that have a readonly ref T Current. For example:

    public struct Enumerator<T> : IEnumerator<T>
    {
        private static readonly DiagNode<T> s_Empty = new DiagNode<T>(default!);

        private DiagNode<T>? _nextNode;
        private DiagNode<T> _currentNode;

        public Enumerator(DiagNode<T>? head)
        {
            _nextNode = head;
            _currentNode = s_Empty;
        }

        public readonly ref T Current => ref _currentNode.Value;

        T IEnumerator<T>.Current => Current;

        object? IEnumerator.Current => Current;

        public bool MoveNext()
        {
            if (_nextNode == null)
            {
                _currentNode =  s_Empty;
                return false;
            }

            _currentNode = _nextNode;
            _nextNode = _nextNode.Next;
            return true;
        }

        public void Reset() => throw new NotSupportedException();

        public void Dispose()
        {
        }
    }

If the ROS option is not viable I will spend more time flushing this out.

Risks

None that I can think of.

Author:	CodeBlanch
Assignees:	-
Labels:	api-suggestion, area-System.Diagnostics.Activity
Milestone:	-

[CodeBlanch]

@tarekgh What I did for my testing was implement it like this in ActivityTagsCollection:

            public ReadOnlySpan<KeyValuePair<string, object?>> AsReadOnlySpan()
            {
                return CollectionsMarshal.AsSpan(_list);
            }

Method	Mean	Error	StdDev	Allocated
TagEnumeration	24.012 ns	0.4830 ns	0.4518 ns	-
TagEnumerationUsingSpan	4.258 ns	0.1148 ns	0.2041 ns	-

Performance is really great! ROS is 🔥 But that only works because ActivityTagsCollection happens to be backed by a list/array.

So if this perf is compelling enough, I think we could switch Activity to use some array-based storage instead of DiagLinkedList<T>. But if that is too big a change or can't be done for some reason, the perf using the enumerator with public readonly ref T Current is also good:

Method	Mean	Error	StdDev	Allocated
TagEnumeration	21.112 ns	0.2359 ns	0.2091 ns	-
TagEnumerationWithRef	5.753 ns	0.0399 ns	0.0333 ns	-

[tarekgh]

What I did for my testing was implement it like this in ActivityTagsCollection:

You have used CollectionsMarshal which exist only in .NET 5.0 and up. This will be challenging if we need to support it for ns2.0. Or we'll have to change the implementation to not use collections and use arrays. I am not sure if this is worth the effort here.

So if this perf is compelling enough, I think we could switch Activity to use some array-based storage instead of DiagLinkedList.

The current implementation of DiagLinkedList is safe enough to be used in the enumeration while the list can change. This will not be guaranteed when using collection (e.g. List<T> or arrays as these may need to grow at any time).

I think the best here is exposing Enumerator<T> maybe with different name and not exposing the constructor? and we can optimize the implementation the way we want.