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

Include net.peer.port in ZipkinSpan.RemoteEndpoint.ServiceName #1168

Merged
merged 9 commits into from
Aug 27, 2020
Merged

Include net.peer.port in ZipkinSpan.RemoteEndpoint.ServiceName #1168

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
76136b5
Include net.peer.port in ZipkinSpan.RemoteEndpoint.ServiceName when a…
alanwest Aug 25, 2020
c48b01f
Revert change to CreateTestActivity
alanwest Aug 26, 2020
c208e31
Delete duplicate test suite
alanwest Aug 26, 2020
891051c
Update changelog
alanwest Aug 26, 2020
c88120d
Enable handling of non-string attributes in Zipkin tests
alanwest Aug 26, 2020
b4dd84d
Revert "Enable handling of non-string attributes in Zipkin tests"
alanwest Aug 26, 2020
9ace996
Merge branch 'master' into alanwest/zipkin-remoteendpoint-port
alanwest Aug 26, 2020
b3d6f02
Use ValueTuple for RemoteEndpointCache key
alanwest Aug 26, 2020
f079722
Merge branch 'master' into alanwest/zipkin-remoteendpoint-port
cijothomas Aug 27, 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
51 changes: 43 additions & 8 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 @@ -33,12 +33,10 @@ internal static class ZipkinActivityConversionExtensions
private static readonly Dictionary<string, int> RemoteEndpointServiceNameKeyResolutionDictionary = new Dictionary<string, int>(StringComparer.OrdinalIgnoreCase)
{
[SemanticConventions.AttributePeerService] = 0, // priority 0 (highest).
[SemanticConventions.AttributeNetPeerName] = 1,
[SemanticConventions.AttributeNetPeerIp] = 2,
["peer.hostname"] = 2,
["peer.address"] = 2,
[SemanticConventions.AttributeHttpHost] = 3, // RemoteEndpoint.ServiceName for Http.
[SemanticConventions.AttributeDbInstance] = 3, // RemoteEndpoint.ServiceName for Redis.
["peer.hostname"] = 1,
["peer.address"] = 1,
[SemanticConventions.AttributeHttpHost] = 2, // RemoteEndpoint.ServiceName for Http.
[SemanticConventions.AttributeDbInstance] = 2, // RemoteEndpoint.ServiceName for Redis.
};

private static readonly string InvalidSpanId = default(ActivitySpanId).ToHexString();
Expand Down Expand Up @@ -96,9 +94,24 @@ internal static ZipkinSpan ToZipkinSpan(this Activity activity, ZipkinEndpoint d
}

ZipkinEndpoint remoteEndpoint = null;
if ((activity.Kind == ActivityKind.Client || activity.Kind == ActivityKind.Producer) && attributeEnumerationState.RemoteEndpointServiceName != null)
if (activity.Kind == ActivityKind.Client || activity.Kind == ActivityKind.Producer)
{
remoteEndpoint = RemoteEndpointCache.GetOrAdd(attributeEnumerationState.RemoteEndpointServiceName, ZipkinEndpoint.Create);
var hostNameOrIpAddress = attributeEnumerationState.HostName ?? attributeEnumerationState.IpAddress;

if ((attributeEnumerationState.RemoteEndpointServiceName == null || attributeEnumerationState.RemoteEndpointServiceNamePriority > 0)
&& hostNameOrIpAddress != null)
{
var remoteEndpointStr = attributeEnumerationState.Port != default
Copy link
Member

Choose a reason for hiding this comment

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

@alanwest It would be great if we didn't need this string allocation. Check out #1153. It's using ValueTuple for the dictionary key. If we made the key ValueTuple<string, int?> we might be able to avoid it?

Copy link
Member Author

Choose a reason for hiding this comment

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

@CodeBlanch I'm not opposed to using a caching mechanism here to avoid string allocations, but I want to be sure to avoid any risk of unbounded memory growth of the cache. I could make a cache local to an individual export operation, but that might limit any perf gain.

It's not immediately apparent to me if the StackExchangeRedisCallsInstrumentation avoids unbounded memory growth. What, if anything, ever clears items from the cache?

Copy link
Member

Choose a reason for hiding this comment

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

@alanwest The one in Redis should only be caching live traces/spans. They should remove as they complete. This cache here, will grow unbounded. It is cheating/assuming that the user will only ever connect to a reasonable set of remote services. That assumption may or may not be accurate, and we might want to add some expiration? But moving from a string key to a ValueTuple doesn't really change that 😄

Copy link
Member Author

@alanwest alanwest Aug 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The one in Redis should only be caching live traces/spans. They should remove as they complete.

Hmm... not seeing where this happens - I'm not very familiar with the Redis instrumentation. Though, I will parking-lot this concern since it's not related to this PR.

It is cheating/assuming that the user will only ever connect to a reasonable set of remote services.

I think it's safer to not make this assumption. I think it is often the case that the set remote services is of a reasonable size, but I've seen numerous instances in customers applications where this set can be very large. This can be common in mutli-tenant architectures where number of hostnames can be large and hostname is used to route to a specific tenant. I think it might make sense to come back to the thought of a cache expiration in a separate issue.

But moving from a string key to a ValueTuple doesn't really change that

Agreed, I've made this change since it seems like a solid approach. Note that the net452 build still uses a string as a key. Could have made this a regular Tuple, but I figured trading a string allocation for a Tuple allocation for net452 didn't make a lot of sense.

? $"{hostNameOrIpAddress}:{attributeEnumerationState.Port}"
: hostNameOrIpAddress;

remoteEndpoint = RemoteEndpointCache.GetOrAdd(remoteEndpointStr, ZipkinEndpoint.Create);
}

if (remoteEndpoint == null && attributeEnumerationState.RemoteEndpointServiceName != null)
{
remoteEndpoint = RemoteEndpointCache.GetOrAdd(attributeEnumerationState.RemoteEndpointServiceName, ZipkinEndpoint.Create);
}
}

var annotations = PooledList<ZipkinAnnotation>.Create();
Expand Down Expand Up @@ -162,6 +175,18 @@ internal static bool ProcessTags(ref AttributeEnumerationState state, KeyValuePa
state.RemoteEndpointServiceName = strVal;
state.RemoteEndpointServiceNamePriority = priority;
}
else if (key == SemanticConventions.AttributeNetPeerName)
{
state.HostName = strVal;
}
else if (key == SemanticConventions.AttributeNetPeerIp)
{
state.IpAddress = strVal;
}
else if (key == SemanticConventions.AttributeNetPeerPort && int.TryParse(strVal, out var port))
{
state.Port = port;
}
else if (key == Resource.ServiceNameKey)
{
state.ServiceName = strVal;
Expand All @@ -175,6 +200,10 @@ internal static bool ProcessTags(ref AttributeEnumerationState state, KeyValuePa
PooledList<KeyValuePair<string, object>>.Add(ref state.Tags, new KeyValuePair<string, object>(key, strVal));
}
}
else if (attribute.Value is int intVal && attribute.Key == SemanticConventions.AttributeNetPeerPort)
{
state.Port = intVal;
}
else
{
PooledList<KeyValuePair<string, object>>.Add(ref state.Tags, attribute);
Expand Down Expand Up @@ -224,6 +253,12 @@ internal struct AttributeEnumerationState
public string ServiceName;

public string ServiceNamespace;

public string HostName;

public string IpAddress;

public int Port;
}
}
}
142 changes: 132 additions & 10 deletions ...lemetry.Exporter.Zipkin.Tests/Implementation/ZipkinActivityExporterRemoteEndpointTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

using System.Collections.Generic;
using OpenTelemetry.Exporter.Zipkin.Implementation;
using OpenTelemetry.Trace;
using Xunit;

namespace OpenTelemetry.Exporter.Zipkin.Tests.Implementation
Expand Down Expand Up @@ -53,23 +54,144 @@ public void ZipkinSpanConverterTest_GenerateActivity_RemoteEndpointResolution()
Assert.Equal("RemoteServiceName", zipkinSpan.RemoteEndpoint.ServiceName);
}

[Fact]
public void ZipkinSpanConverterTest_GenerateActivity_RemoteEndpointResolutionPriority()
[Theory]
[MemberData(nameof(RemoteEndpointPriorityTestCase.GetTestCases), MemberType = typeof(RemoteEndpointPriorityTestCase))]
public void ZipkinSpanConverterTest_GenerateActivity_RemoteEndpointResolutionPriority(RemoteEndpointPriorityTestCase testCase)
{
// Arrange
var activity = ZipkinExporterTests.CreateTestActivity(
additionalAttributes: new Dictionary<string, object>
{
["http.host"] = "DiscardedRemoteServiceName",
["net.peer.name"] = "RemoteServiceName",
["peer.hostname"] = "DiscardedRemoteServiceName",
});
var activity = ZipkinExporterTests.CreateTestActivity(additionalAttributes: testCase.RemoteEndpointAttributes);

// Act & Assert
var zipkinSpan = ZipkinActivityConversionExtensions.ToZipkinSpan(activity, DefaultZipkinEndpoint);

Assert.NotNull(zipkinSpan.RemoteEndpoint);
Assert.Equal("RemoteServiceName", zipkinSpan.RemoteEndpoint.ServiceName);
Assert.Equal(testCase.ExpectedResult, zipkinSpan.RemoteEndpoint.ServiceName);
}

public class RemoteEndpointPriorityTestCase
{
public string Name { get; set; }

public string ExpectedResult { get; set; }

public Dictionary<string, object> RemoteEndpointAttributes { get; set; }

public static IEnumerable<object[]> GetTestCases()
{
yield return new object[]
{
new RemoteEndpointPriorityTestCase
{
Name = "Highest priority name = net.peer.name",
ExpectedResult = "RemoteServiceName",
RemoteEndpointAttributes = new Dictionary<string, object>
{
["http.host"] = "DiscardedRemoteServiceName",
["net.peer.name"] = "RemoteServiceName",
["peer.hostname"] = "DiscardedRemoteServiceName",
},
},
};

yield return new object[]
{
new RemoteEndpointPriorityTestCase
{
Name = "Highest priority name = SemanticConventions.AttributePeerService",
ExpectedResult = "RemoteServiceName",
RemoteEndpointAttributes = new Dictionary<string, object>
{
[SemanticConventions.AttributePeerService] = "RemoteServiceName",
["http.host"] = "DiscardedRemoteServiceName",
["net.peer.name"] = "DiscardedRemoteServiceName",
["net.peer.port"] = "1234",
["peer.hostname"] = "DiscardedRemoteServiceName",
},
},
};

yield return new object[]
{
new RemoteEndpointPriorityTestCase
{
Name = "Only has net.peer.name and net.peer.port",
ExpectedResult = "RemoteServiceName:1234",
RemoteEndpointAttributes = new Dictionary<string, object>
{
["net.peer.name"] = "RemoteServiceName",
["net.peer.port"] = "1234",
},
},
};

yield return new object[]
{
new RemoteEndpointPriorityTestCase
{
Name = "net.peer.port is an int",
ExpectedResult = "RemoteServiceName:1234",
RemoteEndpointAttributes = new Dictionary<string, object>
{
["net.peer.name"] = "RemoteServiceName",
["net.peer.port"] = 1234,
},
},
};

yield return new object[]
{
new RemoteEndpointPriorityTestCase
{
Name = "Has net.peer.name and net.peer.port",
ExpectedResult = "RemoteServiceName:1234",
RemoteEndpointAttributes = new Dictionary<string, object>
{
["http.host"] = "DiscardedRemoteServiceName",
["net.peer.name"] = "RemoteServiceName",
["net.peer.port"] = "1234",
["peer.hostname"] = "DiscardedRemoteServiceName",
},
},
};

yield return new object[]
{
new RemoteEndpointPriorityTestCase
{
Name = "Has net.peer.ip and net.peer.port",
ExpectedResult = "1.2.3.4:1234",
RemoteEndpointAttributes = new Dictionary<string, object>
{
["http.host"] = "DiscardedRemoteServiceName",
["net.peer.ip"] = "1.2.3.4",
["net.peer.port"] = "1234",
["peer.hostname"] = "DiscardedRemoteServiceName",
},
},
};

yield return new object[]
{
new RemoteEndpointPriorityTestCase
{
Name = "Has net.peer.name, net.peer.ip, and net.peer.port",
ExpectedResult = "RemoteServiceName:1234",
RemoteEndpointAttributes = new Dictionary<string, object>
{
["http.host"] = "DiscardedRemoteServiceName",
["net.peer.name"] = "RemoteServiceName",
["net.peer.ip"] = "1.2.3.4",
["net.peer.port"] = "1234",
["peer.hostname"] = "DiscardedRemoteServiceName",
},
},
};
}

public override string ToString()
{
return this.Name;
}
}
}
}
4 changes: 2 additions & 2 deletions test/OpenTelemetry.Exporter.Zipkin.Tests/ZipkinExporterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ public void ZipkinExporterIntegrationTest(bool useShortTraceIds)
var traceId = useShortTraceIds ? TraceId.Substring(TraceId.Length - 16, 16) : TraceId;

Assert.Equal(
$@"[{{""traceId"":""{traceId}"",""name"":""Name"",""parentId"":""{ZipkinActivityConversionExtensions.EncodeSpanId(activity.ParentSpanId)}"",""id"":""{ZipkinActivityConversionExtensions.EncodeSpanId(context.SpanId)}"",""kind"":""CLIENT"",""timestamp"":{timestamp},""duration"":60000000,""localEndpoint"":{{""serviceName"":""OpenTelemetry Exporter""{ipInformation}}},""annotations"":[{{""timestamp"":{eventTimestamp},""value"":""Event1""}},{{""timestamp"":{eventTimestamp},""value"":""Event2""}}],""tags"":{{""stringKey"":""value"",""longKey"":""1"",""longKey2"":""1"",""doubleKey"":""1"",""doubleKey2"":""1"",""boolKey"":""True"",""library.name"":""CreateTestActivity""}}}}]",
$@"[{{""traceId"":""{traceId}"",""name"":""Name"",""parentId"":""{ZipkinActivityConversionExtensions.EncodeSpanId(activity.ParentSpanId)}"",""id"":""{ZipkinActivityConversionExtensions.EncodeSpanId(context.SpanId)}"",""kind"":""CLIENT"",""timestamp"":{timestamp},""duration"":60000000,""localEndpoint"":{{""serviceName"":""OpenTelemetry Exporter""{ipInformation}}},""annotations"":[{{""timestamp"":{eventTimestamp},""value"":""Event1""}},{{""timestamp"":{eventTimestamp},""value"":""Event2""}}],""tags"":{{""stringKey"":""value"",""longKey"":1,""longKey2"":1,""doubleKey"":1.0,""doubleKey2"":1.0,""boolKey"":true,""library.name"":""CreateTestActivity""}}}}]",
alanwest marked this conversation as resolved.
Show resolved Hide resolved
Responses[requestId]);
}

Expand Down Expand Up @@ -229,7 +229,7 @@ internal static Activity CreateTestActivity(
var activitySource = new ActivitySource(nameof(CreateTestActivity));

var tags = setAttributes ?
attributes.Select(kvp => new KeyValuePair<string, object>(kvp.Key, kvp.Value.ToString()))
attributes.Select(kvp => new KeyValuePair<string, object>(kvp.Key, kvp.Value))
: null;
var links = addLinks ?
new[]
Expand Down