Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added EnumerateTagValues Activity Extension #1236

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/OpenTelemetry.Api/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
* Added `GetTagValue` extension method on `Activity` for retrieving tag values
efficiently
([#1221](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1221))
* Added `EnumerateTagValues` extension method on `Activity` for enumerating tag
values efficiently
([#1236](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1236))

## 0.5.0-beta.2

Expand Down
119 changes: 93 additions & 26 deletions src/OpenTelemetry.Api/Trace/ActivityExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,6 @@ public static class ActivityExtensions
{
private static readonly object EmptyActivityTagObjects = typeof(Activity).GetField("s_emptyTagObjects", BindingFlags.Static | BindingFlags.NonPublic).GetValue(null);

private static readonly Enumerator<IEnumerable<KeyValuePair<string, object>>, KeyValuePair<string, object>, KeyValuePair<string, object>>.AllocationFreeForEachDelegate
ActivityTagObjectsEnumerator = DictionaryEnumerator<string, object, KeyValuePair<string, object>>.BuildAllocationFreeForEachDelegate(
typeof(Activity).GetField("_tags", BindingFlags.Instance | BindingFlags.NonPublic).FieldType);

private static readonly DictionaryEnumerator<string, object, KeyValuePair<string, object>>.ForEachDelegate GetTagValueCallbackRef = GetTagValueCallback;

/// <summary>
/// Sets the status of activity execution.
/// Activity class in .NET does not support 'Status'.
Expand Down Expand Up @@ -70,14 +64,15 @@ public static Status GetStatus(this Activity activity)
{
Debug.Assert(activity != null, "Activity should not be null");

var statusCanonicalCode = activity.GetTagValue(SpanAttributeConstants.StatusCodeKey) as string;
var statusDescription = activity.GetTagValue(SpanAttributeConstants.StatusDescriptionKey) as string;
ActivityStatusTagEnumerator state = default;

ActivityTagObjectsEnumeratorFactory<ActivityStatusTagEnumerator>.Enumerate(activity, ref state);

var status = SpanHelper.ResolveCanonicalCodeToStatus(statusCanonicalCode);
var status = SpanHelper.ResolveCanonicalCodeToStatus(state.StatusCode);

if (status.IsValid && !string.IsNullOrEmpty(statusDescription))
if (status.IsValid && !string.IsNullOrEmpty(state.StatusDescription))
{
return status.WithDescription(statusDescription);
return status.WithDescription(state.StatusDescription);
}

return status;
Expand All @@ -92,22 +87,30 @@ public static Status GetStatus(this Activity activity)
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static object GetTagValue(this Activity activity, string tagName)
{
var tagObjects = activity.TagObjects;
if (ReferenceEquals(tagObjects, EmptyActivityTagObjects))
{
return null;
}
Debug.Assert(activity != null, "Activity should not be null");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: consider the stock Assert.IsNotNull.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want me to add Xunit to API? 🤣 I don't think there's a similar method on System.Diagnostics.Debug, but there should be!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not familiar with .NET, I thought Assert.IsNotNull is a common thing and am surprised that it is not (so it looks like a compile time trick).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry didn't mean to poke fun 😄 Ya it's common to unit test frameworks but I don't think there's anything in the BCL.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a look at the doc and it makes me wonder if we should use it or throw normal exception.

It seems in C++ the general trend is to avoid https://en.cppreference.com/w/c/error/assert from a library.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can switch them to regular null checks if you would like. My thinking with the Debug.Assert was... user will have to work really hard to call these extensions with a null Activity. You have to go out of your way and do OpenTelemetry.Trace.ActivityExtensions.GetTagValue(null, "myTag"). If you are set on shooting yourself in the foot, who are we to stop you? 🤣 But for the 99.99% of callers who are doing activity.GetTagValue("myTag") it really will never be null, so I was trying to save the perf of doing an unnecessary check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to get your perspective and see if I need to change this

.

Based on the discussion, I think we should remove the null check for extension methods?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is kind of a risk/reward thing. That particular extension would only be called a handful of times (?) so it's probably fine to leave the check in there. If we don't have the check, static analysis tools like FxCop get upset so then we end up with suppressions which are kind of a maintainability headache. IMO we should only do this on hot-path stuff where we know there is a compelling benefit to justify the suppression. Assuming we get to turning FxCop on 😄 Poor @eddynaka has been trying to do that for a while.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its getting harder and harder :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coming into the middle of this conversation, though reminds me of System.Diagnostics.Contracts.


KeyValuePair<string, object> state = new KeyValuePair<string, object>(tagName, null);
ActivitySingleTagEnumerator state = new ActivitySingleTagEnumerator(tagName);

ActivityTagObjectsEnumerator(
tagObjects,
ref state,
GetTagValueCallbackRef);
ActivityTagObjectsEnumeratorFactory<ActivitySingleTagEnumerator>.Enumerate(activity, ref state);

return state.Value;
}

/// <summary>
/// Enumerates all the key/value pairs on an <see cref="Activity"/> without performing an allocation.
/// </summary>
/// <typeparam name="T">The struct <see cref="IActivityTagEnumerator"/> implementation to use for the enumeration.</typeparam>
/// <param name="activity">Activity instance.</param>
/// <param name="tagEnumerator">Tag enumerator.</param>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static void EnumerateTagValues<T>(this Activity activity, ref T tagEnumerator)
where T : struct, IActivityTagEnumerator
{
Debug.Assert(activity != null, "Activity should not be null");

ActivityTagObjectsEnumeratorFactory<T>.Enumerate(activity, ref tagEnumerator);
}

/// <summary>
/// Record Exception.
/// </summary>
Expand Down Expand Up @@ -135,15 +138,79 @@ public static void RecordException(this Activity activity, Exception ex)
activity?.AddEvent(new ActivityEvent(SemanticConventions.AttributeExceptionEventName, default, tagsCollection));
}

private static bool GetTagValueCallback(ref KeyValuePair<string, object> state, KeyValuePair<string, object> item)
private struct ActivitySingleTagEnumerator : IActivityTagEnumerator
{
if (item.Key == state.Key)
private readonly string tagName;

public ActivitySingleTagEnumerator(string tagName)
{
state = item;
return false;
this.tagName = tagName;
this.Value = null;
}

public object Value { get; private set; }

public bool ForEach(KeyValuePair<string, object> item)
{
if (item.Key == this.tagName)
{
this.Value = item.Value;
return false;
}

return true;
}
}

private struct ActivityStatusTagEnumerator : IActivityTagEnumerator
{
public string StatusCode { get; private set; }

public string StatusDescription { get; private set; }

public bool ForEach(KeyValuePair<string, object> item)
{
switch (item.Key)
{
case SpanAttributeConstants.StatusCodeKey:
this.StatusCode = item.Value as string;
break;
case SpanAttributeConstants.StatusDescriptionKey:
this.StatusDescription = item.Value as string;
break;
}

return this.StatusCode == null || this.StatusDescription == null;
}
}

private static class ActivityTagObjectsEnumeratorFactory<TState>
where TState : struct, IActivityTagEnumerator
{
private static readonly DictionaryEnumerator<string, object, TState>.AllocationFreeForEachDelegate
ActivityTagObjectsEnumerator = DictionaryEnumerator<string, object, TState>.BuildAllocationFreeForEachDelegate(
typeof(Activity).GetField("_tags", BindingFlags.Instance | BindingFlags.NonPublic).FieldType);

private static readonly DictionaryEnumerator<string, object, TState>.ForEachDelegate ForEachTagValueCallbackRef = ForEachTagValueCallback;

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static void Enumerate(Activity activity, ref TState state)
{
var tagObjects = activity.TagObjects;

if (ReferenceEquals(tagObjects, EmptyActivityTagObjects))
{
return;
}

ActivityTagObjectsEnumerator(
tagObjects,
ref state,
ForEachTagValueCallbackRef);
}

return true;
private static bool ForEachTagValueCallback(ref TState state, KeyValuePair<string, object> item)
=> state.ForEach(item);
}
}
}
34 changes: 34 additions & 0 deletions src/OpenTelemetry.Api/Trace/IActivityTagEnumerator.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// <copyright file="IActivityTagEnumerator.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System.Collections.Generic;
using System.Diagnostics;

namespace OpenTelemetry.Trace
{
/// <summary>
/// An interface used to perform zero-allocation enumeration of <see cref="Activity"/> tags. Implementation must be a struct.
/// </summary>
public interface IActivityTagEnumerator
{
/// <summary>
/// Called for each <see cref="Activity"/> tag while the enumeration is executing.
/// </summary>
/// <param name="item">Tag key/value pair.</param>
/// <returns><see langword="true"/> to continue the enumeration of records or <see langword="false"/> to stop (break) the enumeration.</returns>
bool ForEach(KeyValuePair<string, object> item);
}
}
46 changes: 40 additions & 6 deletions test/Benchmarks/ActivityBenchmarks.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,15 @@ static ActivityBenchmarks()
Activity.AddTag("Tag1", "Value1");
Activity.AddTag("Tag2", 2);
Activity.AddTag("Tag3", false);

for (int i = 0; i < 1024; i++)
{
Activity.AddTag($"AutoTag{i}", i);
}
}

[Benchmark]
public void EnumerateEmptyTagObjects()
public void SearchEnumerateEmptyTagObjects()
{
object value;
foreach (KeyValuePair<string, object> tag in EmptyActivity.TagObjects)
Expand All @@ -53,19 +58,19 @@ public void EnumerateEmptyTagObjects()
}

[Benchmark]
public void LinqEmptyTagObjects()
public void SearchLinqEmptyTagObjects()
{
EmptyActivity.TagObjects.FirstOrDefault(i => i.Key == "Tag3");
}

[Benchmark]
public void GetTagValueEmptyTagObjects()
public void SearchGetTagValueEmptyTagObjects()
{
EmptyActivity.GetTagValue("Tag3");
}

[Benchmark]
public void EnumerateNonemptyTagObjects()
public void SearchEnumerateNonemptyTagObjects()
{
object value;
foreach (KeyValuePair<string, object> tag in Activity.TagObjects)
Expand All @@ -79,15 +84,44 @@ public void EnumerateNonemptyTagObjects()
}

[Benchmark]
public void LinqNonemptyTagObjects()
public void SearchLinqNonemptyTagObjects()
{
Activity.TagObjects.FirstOrDefault(i => i.Key == "Tag3");
}

[Benchmark]
public void GetTagValueNonemptyTagObjects()
public void SearchGetTagValueNonemptyTagObjects()
{
Activity.GetTagValue("Tag3");
}

[Benchmark]
public void EnumerateNonemptyTagObjects()
{
int count = 0;
foreach (KeyValuePair<string, object> tag in Activity.TagObjects)
{
count++;
}
}

[Benchmark]
public void EnumerateTagValuesNonemptyTagObjects()
{
Enumerator state = default;

Activity.EnumerateTagValues(ref state);
}

private struct Enumerator : IActivityTagEnumerator
{
public int Count { get; private set; }

public bool ForEach(KeyValuePair<string, object> item)
{
this.Count++;
return true;
}
}
}
}
72 changes: 72 additions & 0 deletions test/OpenTelemetry.Tests/Trace/ActivityExtensionsTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
// </copyright>

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using Xunit;
Expand Down Expand Up @@ -134,5 +135,76 @@ public void GetTagValue()
Assert.Null(activity.GetTagValue("tag1"));
Assert.Null(activity.GetTagValue("Tag2"));
}

[Fact]
public void EnumerateTagValuesEmpty()
{
Activity activity = new Activity("Test");

Enumerator state = default;

activity.EnumerateTagValues(ref state);

Assert.Equal(0, state.Count);
Assert.False(state.LastTag.HasValue);
}

[Fact]
public void EnumerateTagValuesAll()
{
Activity activity = new Activity("Test");
activity.AddTag("Tag1", "Value1");
activity.AddTag("Tag2", "Value2");
activity.AddTag("Tag3", "Value3");

Enumerator state = default;

activity.EnumerateTagValues(ref state);

Assert.Equal(3, state.Count);
Assert.True(state.LastTag.HasValue);
Assert.Equal("Tag3", state.LastTag?.Key);
Assert.Equal("Value3", state.LastTag?.Value);
}

[Fact]
public void EnumerateTagValuesWithBreak()
{
Activity activity = new Activity("Test");
activity.AddTag("Tag1", "Value1");
activity.AddTag("Tag2", "Value2");
activity.AddTag("Tag3", "Value3");

Enumerator state = default;
state.BreakOnCount = 1;

activity.EnumerateTagValues(ref state);

Assert.Equal(1, state.Count);
Assert.True(state.LastTag.HasValue);
Assert.Equal("Tag1", state.LastTag?.Key);
Assert.Equal("Value1", state.LastTag?.Value);
}

private struct Enumerator : IActivityTagEnumerator
{
public int BreakOnCount { get; set; }

public KeyValuePair<string, object>? LastTag { get; private set; }

public int Count { get; private set; }

public bool ForEach(KeyValuePair<string, object> item)
{
this.LastTag = item;
this.Count++;
if (this.BreakOnCount == this.Count)
{
return false;
}

return true;
}
}
}
}