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.

Sign up for GitHub

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

Updating exporters to use TagObjects instead of Tags #1000

Merged
merged 8 commits into from
Aug 5, 2020
Merged

Updating exporters to use TagObjects instead of Tags #1000

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
967ddc3
Update ZipkinExporter to use TagObjects instead of Tags
eddynaka Aug 4, 2020
4afde9d
Updating JaegerExporter to use TagObjects
eddynaka Aug 4, 2020
01ef4f4
Updating OtlpExporter to use TagObjects
eddynaka Aug 4, 2020
2e2dc19
removing unused using
eddynaka Aug 4, 2020
886ec49
Removing duplicated logic
eddynaka Aug 4, 2020
b70b799
Merge branch 'master' into feature/updating-zipking-newactivity
eddynaka Aug 4, 2020
e8f94f5
Merge branch 'master' into feature/updating-zipking-newactivity
eddynaka Aug 4, 2020
2cb7d48
Merge branch 'master' into feature/updating-zipking-newactivity
cijothomas Aug 4, 2020
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
39 changes: 16 additions & 23 deletions src/OpenTelemetry.Exporter.Jaeger/Implementation/JaegerActivityExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,11 @@ internal static class JaegerActivityExtensions
[SemanticConventions.AttributeNetPeerIp] = 2,
["peer.hostname"] = 2,
["peer.address"] = 2,
["http.host"] = 3, // peer.service for Http.
["db.instance"] = 3, // peer.service for Redis.
[SemanticConventions.AttributeHttpHost] = 3, // peer.service for Http.
[SemanticConventions.AttributeDbInstance] = 3, // peer.service for Redis.
};

private static readonly DictionaryEnumerator<string, string, TagState>.ForEachDelegate ProcessActivityTagRef = ProcessActivityTag;
private static readonly DictionaryEnumerator<string, object, TagState>.ForEachDelegate ProcessActivityTagRef = ProcessActivityTag;
private static readonly ListEnumerator<ActivityLink, PooledListState<JaegerSpanRef>>.ForEachDelegate ProcessActivityLinkRef = ProcessActivityLink;
private static readonly ListEnumerator<ActivityEvent, PooledListState<JaegerLog>>.ForEachDelegate ProcessActivityEventRef = ProcessActivityEvent;
private static readonly DictionaryEnumerator<string, object, PooledListState<JaegerTag>>.ForEachDelegate ProcessTagRef = ProcessTag;
Expand All @@ -64,8 +64,8 @@ public static JaegerSpan ToJaegerSpan(this Activity activity)
Tags = PooledList<JaegerTag>.Create(),
};

DictionaryEnumerator<string, string, TagState>.AllocationFreeForEach(
activity.Tags,
DictionaryEnumerator<string, object, TagState>.AllocationFreeForEach(
activity.TagObjects,
Copy link
Member

Choose a reason for hiding this comment

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

@tarekgh Just noticing Activity.TagObjects uses a custom LinkedList and not ActivityTagsCollection like ActivityEvent & ActivityLink use. Because of that, no struct enumerator, perf regression for us anytime we enumerate TagObjects. Is there a compelling reason for the difference? What we really need is for whatever is underneath the IEnumerable returned to have a struct GetEnumerator() operation so we can foreach without allocation.

Something like...

   private class TagsLinkedList : IEnumerable<KeyValuePair<string, object>>
   {
        public Enumerator<KeyValuePair<string, object>> GetEnumerator() { return Enumerator(this); }
        IEnumerator<T> IEnumerable<KeyValuePair<string, object>>.GetEnumerator() { return Enumerator(this); }
        IEnumerator IEnumerable.GetEnumerator() { return Enumerator(this); }

        private struct Enumerator<KeyValuePair<string, object>> { ... }
   }

        public IEnumerable<KeyValuePair<string, object?>> TagObjects
        {
            get => _tags ?? Unsafe.As<IEnumerable<KeyValuePair<string, object?>>>(s_emptyBaggageTags);
        }

Copy link
Contributor

Choose a reason for hiding this comment

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

This is something can look at to optimize as everything is implementation details. I didn't change anything from what we used to have. Note that this will be a little bit tricky because it is possible some tags get removed while someone else enumerating the list.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome this would help a lot. When I say perf regression I mean moving from our old Span object to using Activity to drive everything.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you are interested, you may submit a PR for that? we can work out the details together if you want.

CC @noahfalk

Copy link
Member

Choose a reason for hiding this comment

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

@tarekgh Done dotnet/runtime#40362

ref jaegerTags,
ProcessActivityTagRef);

Expand Down Expand Up @@ -217,23 +217,16 @@ public static JaegerSpanRef ToJaegerSpanRef(this in ActivityLink link)

public static JaegerTag ToJaegerTag(this KeyValuePair<string, object> attribute)
{
switch (attribute.Value)
return attribute.Value switch
{
case string s:
return new JaegerTag(attribute.Key, JaegerTagType.STRING, vStr: s);
case int i:
return new JaegerTag(attribute.Key, JaegerTagType.LONG, vLong: Convert.ToInt64(i));
case long l:
return new JaegerTag(attribute.Key, JaegerTagType.LONG, vLong: l);
case float f:
return new JaegerTag(attribute.Key, JaegerTagType.DOUBLE, vDouble: Convert.ToDouble(f));
case double d:
return new JaegerTag(attribute.Key, JaegerTagType.DOUBLE, vDouble: d);
case bool b:
return new JaegerTag(attribute.Key, JaegerTagType.BOOL, vBool: b);
}

return new JaegerTag(attribute.Key, JaegerTagType.STRING, vStr: attribute.Value.ToString());
string s => new JaegerTag(attribute.Key, JaegerTagType.STRING, vStr: s),
Copy link
Member

Choose a reason for hiding this comment

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

we now need to deal with the array of primitives since #999 is merged.

int i => new JaegerTag(attribute.Key, JaegerTagType.LONG, vLong: Convert.ToInt64(i)),
long l => new JaegerTag(attribute.Key, JaegerTagType.LONG, vLong: l),
float f => new JaegerTag(attribute.Key, JaegerTagType.DOUBLE, vDouble: Convert.ToDouble(f)),
double d => new JaegerTag(attribute.Key, JaegerTagType.DOUBLE, vDouble: d),
bool b => new JaegerTag(attribute.Key, JaegerTagType.BOOL, vBool: b),
_ => new JaegerTag(attribute.Key, JaegerTagType.STRING, vStr: attribute.Value.ToString()),
};
}

public static long ToEpochMicroseconds(this DateTime utcDateTime)
Expand All @@ -256,9 +249,9 @@ public static long ToEpochMicroseconds(this DateTimeOffset timestamp)
return microseconds - UnixEpochMicroseconds;
}

private static bool ProcessActivityTag(ref TagState state, KeyValuePair<string, string> activityTag)
private static bool ProcessActivityTag(ref TagState state, KeyValuePair<string, object> activityTag)
{
var jaegerTag = new JaegerTag(activityTag.Key, JaegerTagType.STRING, activityTag.Value);
JaegerTag jaegerTag = activityTag.ToJaegerTag();

if (jaegerTag.VStr != null
&& PeerServiceKeyResolutionDictionary.TryGetValue(activityTag.Key, out int priority)
Expand Down
35 changes: 1 addition & 34 deletions src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/ActivityExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ internal static OtlpTrace.Span ToOtlpSpan(this Activity activity)
EndTimeUnixNano = (ulong)(startTimeUnixNano + activity.Duration.ToNanoseconds()),
};

foreach (var kvp in activity.Tags)
foreach (var kvp in activity.TagObjects)
{
var attribute = ToOtlpAttribute(kvp);
if (attribute != null && attribute.Key != SpanAttributeConstants.StatusCodeKey && attribute.Key != SpanAttributeConstants.StatusDescriptionKey)
Expand Down Expand Up @@ -219,39 +219,6 @@ private static OtlpTrace.Span.Types.Event ToOtlpEvent(ActivityEvent activityEven
return otlpEvent;
}

private static OtlpCommon.KeyValue ToOtlpAttribute(KeyValuePair<string, string> kvp)
{
// TODO: enforce no duplicate keys?
// TODO: reverse?
// To maintain full fidelity to downstream receivers convert to the proper attribute types

if (kvp.Value == null)
{
return null;
}

var attrib = new OtlpCommon.KeyValue { Key = kvp.Key, Value = new OtlpCommon.AnyValue { } };

if (long.TryParse(kvp.Value, out var longValue))
{
attrib.Value.IntValue = longValue;
}
else if (double.TryParse(kvp.Value, out var doubleValue))
{
attrib.Value.DoubleValue = doubleValue;
}
else if (bool.TryParse(kvp.Value, out var boolValue))
{
attrib.Value.BoolValue = boolValue;
}
else
{
attrib.Value.StringValue = kvp.Value;
}

return attrib;
}

private static OtlpCommon.KeyValue ToOtlpAttribute(KeyValuePair<string, object> kvp)
{
if (kvp.Value == null)
Expand Down
43 changes: 19 additions & 24 deletions src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,16 @@ internal static class ZipkinActivityConversionExtensions
[SemanticConventions.AttributeNetPeerIp] = 2,
["peer.hostname"] = 2,
["peer.address"] = 2,
["http.host"] = 3, // RemoteEndpoint.ServiceName for Http.
["db.instance"] = 3, // RemoteEndpoint.ServiceName for Redis.
[SemanticConventions.AttributeHttpHost] = 3, // RemoteEndpoint.ServiceName for Http.
[SemanticConventions.AttributeDbInstance] = 3, // RemoteEndpoint.ServiceName for Redis.
};

private static readonly string InvalidSpanId = default(ActivitySpanId).ToHexString();

private static readonly ConcurrentDictionary<string, ZipkinEndpoint> LocalEndpointCache = new ConcurrentDictionary<string, ZipkinEndpoint>();
private static readonly ConcurrentDictionary<string, ZipkinEndpoint> RemoteEndpointCache = new ConcurrentDictionary<string, ZipkinEndpoint>();

private static readonly DictionaryEnumerator<string, string, AttributeEnumerationState>.ForEachDelegate ProcessTagsRef = ProcessTags;
private static readonly DictionaryEnumerator<string, object, AttributeEnumerationState>.ForEachDelegate ProcessTagsRef = ProcessTags;
private static readonly ListEnumerator<ActivityEvent, PooledList<ZipkinAnnotation>>.ForEachDelegate ProcessActivityEventsRef = ProcessActivityEvents;

internal static ZipkinSpan ToZipkinSpan(this Activity activity, ZipkinEndpoint defaultLocalEndpoint, bool useShortTraceIds = false)
Expand All @@ -62,18 +62,18 @@ internal static ZipkinSpan ToZipkinSpan(this Activity activity, ZipkinEndpoint d

var attributeEnumerationState = new AttributeEnumerationState
{
Tags = PooledList<KeyValuePair<string, string>>.Create(),
Tags = PooledList<KeyValuePair<string, object>>.Create(),
};

DictionaryEnumerator<string, string, AttributeEnumerationState>.AllocationFreeForEach(activity.Tags, ref attributeEnumerationState, ProcessTagsRef);
DictionaryEnumerator<string, object, AttributeEnumerationState>.AllocationFreeForEach(activity.TagObjects, ref attributeEnumerationState, ProcessTagsRef);

var activitySource = activity.Source;
if (!string.IsNullOrEmpty(activitySource.Name))
{
PooledList<KeyValuePair<string, string>>.Add(ref attributeEnumerationState.Tags, new KeyValuePair<string, string>("library.name", activitySource.Name));
PooledList<KeyValuePair<string, object>>.Add(ref attributeEnumerationState.Tags, new KeyValuePair<string, object>("library.name", activitySource.Name));
if (!string.IsNullOrEmpty(activitySource.Version))
{
PooledList<KeyValuePair<string, string>>.Add(ref attributeEnumerationState.Tags, new KeyValuePair<string, string>("library.version", activitySource.Version));
PooledList<KeyValuePair<string, object>>.Add(ref attributeEnumerationState.Tags, new KeyValuePair<string, object>("library.version", activitySource.Version));
}
}

Expand Down Expand Up @@ -161,19 +161,14 @@ private static string EncodeTraceId(ActivityTraceId traceId, bool useShortTraceI

private static string ToActivityKind(Activity activity)
{
switch (activity.Kind)
return activity.Kind switch
{
case ActivityKind.Server:
return "SERVER";
case ActivityKind.Producer:
return "PRODUCER";
case ActivityKind.Consumer:
return "CONSUMER";
case ActivityKind.Client:
return "CLIENT";
}

return null;
ActivityKind.Server => "SERVER",
ActivityKind.Producer => "PRODUCER",
ActivityKind.Consumer => "CONSUMER",
ActivityKind.Client => "CLIENT",
_ => null,
};
}

private static bool ProcessActivityEvents(ref PooledList<ZipkinAnnotation> annotations, ActivityEvent @event)
Expand All @@ -182,10 +177,10 @@ private static bool ProcessActivityEvents(ref PooledList<ZipkinAnnotation> annot
return true;
}

private static bool ProcessTags(ref AttributeEnumerationState state, KeyValuePair<string, string> attribute)
private static bool ProcessTags(ref AttributeEnumerationState state, KeyValuePair<string, object> attribute)
{
string key = attribute.Key;
string strVal = attribute.Value;
string strVal = attribute.Value as string;

if (strVal != null)
{
Expand All @@ -205,20 +200,20 @@ private static bool ProcessTags(ref AttributeEnumerationState state, KeyValuePai
}
else
{
PooledList<KeyValuePair<string, string>>.Add(ref state.Tags, new KeyValuePair<string, string>(key, strVal));
PooledList<KeyValuePair<string, object>>.Add(ref state.Tags, new KeyValuePair<string, object>(key, strVal));
}
}
else
{
PooledList<KeyValuePair<string, string>>.Add(ref state.Tags, new KeyValuePair<string, string>(key, strVal));
PooledList<KeyValuePair<string, object>>.Add(ref state.Tags, new KeyValuePair<string, object>(key, strVal));
}

return true;
}

private struct AttributeEnumerationState
{
public PooledList<KeyValuePair<string, string>> Tags;
public PooledList<KeyValuePair<string, object>> Tags;

public string RemoteEndpointServiceName;

Expand Down
27 changes: 24 additions & 3 deletions src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinSpan.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public ZipkinSpan(
ZipkinEndpoint localEndpoint,
ZipkinEndpoint remoteEndpoint,
in PooledList<ZipkinAnnotation>? annotations,
in PooledList<KeyValuePair<string, string>>? tags,
in PooledList<KeyValuePair<string, object>>? tags,
bool? debug,
bool? shared)
{
Expand Down Expand Up @@ -86,7 +86,7 @@ public ZipkinSpan(

public PooledList<ZipkinAnnotation>? Annotations { get; }

public PooledList<KeyValuePair<string, string>>? Tags { get; }
public PooledList<KeyValuePair<string, object>>? Tags { get; }

public bool? Debug { get; }

Expand Down Expand Up @@ -282,7 +282,28 @@ public void Write(Utf8JsonWriter writer)

foreach (var tag in this.Tags.Value)
{
writer.WriteString(tag.Key, tag.Value);
if (tag.Value is int intValue)
{
writer.WriteNumber(tag.Key, intValue);
}
else if (tag.Value is bool boolVal)
{
writer.WriteBoolean(tag.Key, boolVal);
}
else if (tag.Value is double doubleVal)
{
writer.WriteNumber(tag.Key, doubleVal);
}
else if (tag.Value is string stringVal)
{
writer.WriteString(tag.Key, stringVal);
}
else
{
// Should we try to convert to string? Or
// just drop it?
writer.WriteString(tag.Key, tag.Value.ToString());
}
}

writer.WriteEndObject();
Expand Down
2 changes: 1 addition & 1 deletion test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpExporterTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ public void ToOtlpSpanTest()

foreach (var kvp in attributes)
{
rootActivity.SetTag(kvp.Key, kvp.Value.ToString());
rootActivity.SetTag(kvp.Key, kvp.Value);
}

var startTime = new DateTime(2020, 02, 20, 20, 20, 20, DateTimeKind.Utc);
Expand Down