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

* Include net.peer.port in ZipkinSpan.RemoteEndpoint.ServiceName when applicable * Revert change to CreateTestActivity * Delete duplicate test suite * Update changelog * Enable handling of non-string attributes in Zipkin tests * Revert "Enable handling of non-string attributes in Zipkin tests" This reverts commit c88120d. * Use ValueTuple for RemoteEndpointCache key Co-authored-by: Cijo Thomas <[email protected]>
open-telemetry · Aug 27, 2020 · 4915d74 · 4915d74
1 parent f48e3b4
commit 4915d74
Show file tree

Hide file tree

Showing 5 changed files with 202 additions and 92 deletions.
diff --git a/src/OpenTelemetry.Exporter.Zipkin/CHANGELOG.md b/src/OpenTelemetry.Exporter.Zipkin/CHANGELOG.md
@@ -6,6 +6,9 @@
   ([#1066](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1066))
 * Changed `ZipkinExporter` to use `BatchExportActivityProcessor` by default
   ([#1103](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1103))
+* Fixed issue when span has both the `net.peer.name` and `net.peer.port`
+  attributes but did not include `net.peer.port` in the service address field
+  ([#1168](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1168)).
 
 ## 0.4.0-beta.2
 

diff --git a/src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs b/src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs
@@ -33,18 +33,21 @@ 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();
 
         private static readonly ConcurrentDictionary<string, ZipkinEndpoint> LocalEndpointCache = new ConcurrentDictionary<string, ZipkinEndpoint>();
+
+#if !NET452
+        private static readonly ConcurrentDictionary<(string, int), ZipkinEndpoint> RemoteEndpointCache = new ConcurrentDictionary<(string, int), ZipkinEndpoint>();
+#else
         private static readonly ConcurrentDictionary<string, ZipkinEndpoint> RemoteEndpointCache = new ConcurrentDictionary<string, ZipkinEndpoint>();
+#endif
 
         private static readonly DictionaryEnumerator<string, object, AttributeEnumerationState>.ForEachDelegate ProcessTagsRef = ProcessTags;
         private static readonly ListEnumerator<ActivityEvent, PooledList<ZipkinAnnotation>>.ForEachDelegate ProcessActivityEventsRef = ProcessActivityEvents;
@@ -96,9 +99,32 @@ 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)
+                {
+#if !NET452
+                    remoteEndpoint = RemoteEndpointCache.GetOrAdd((hostNameOrIpAddress, attributeEnumerationState.Port), ZipkinEndpoint.Create);
+#else
+                    var remoteEndpointStr = attributeEnumerationState.Port != default
+                        ? $"{hostNameOrIpAddress}:{attributeEnumerationState.Port}"
+                        : hostNameOrIpAddress;
+
+                    remoteEndpoint = RemoteEndpointCache.GetOrAdd(remoteEndpointStr, ZipkinEndpoint.Create);
+#endif
+                }
+
+                if (remoteEndpoint == null && attributeEnumerationState.RemoteEndpointServiceName != null)
+                {
+#if !NET452
+                    remoteEndpoint = RemoteEndpointCache.GetOrAdd((attributeEnumerationState.RemoteEndpointServiceName, default), ZipkinEndpoint.Create);
+#else
+                    remoteEndpoint = RemoteEndpointCache.GetOrAdd(attributeEnumerationState.RemoteEndpointServiceName, ZipkinEndpoint.Create);
+#endif
+                }
             }
 
             var annotations = PooledList<ZipkinAnnotation>.Create();
@@ -162,6 +188,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;
@@ -175,6 +213,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);
@@ -224,6 +266,12 @@ internal struct AttributeEnumerationState
             public string ServiceName;
 
             public string ServiceNamespace;
+
+            public string HostName;
+
+            public string IpAddress;
+
+            public int Port;
         }
     }
 }
diff --git a/src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinEndpoint.cs b/src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinEndpoint.cs
@@ -53,6 +53,17 @@ public static ZipkinEndpoint Create(string serviceName)
             return new ZipkinEndpoint(serviceName);
         }
 
+#if !NET452
+        public static ZipkinEndpoint Create((string name, int port) serviceNameAndPort)
+        {
+            var serviceName = serviceNameAndPort.port == default
+                ? serviceNameAndPort.name
+                : $"{serviceNameAndPort.name}:{serviceNameAndPort.port}";
+
+            return new ZipkinEndpoint(serviceName);
+        }
+#endif
+
         public ZipkinEndpoint Clone(string serviceName)
         {
             return new ZipkinEndpoint(

diff --git a/...lemetry.Exporter.Zipkin.Tests/Implementation/ZipkinActivityExporterRemoteEndpointTests.cs b/...lemetry.Exporter.Zipkin.Tests/Implementation/ZipkinActivityExporterRemoteEndpointTests.cs
@@ -16,6 +16,7 @@
 
 using System.Collections.Generic;
 using OpenTelemetry.Exporter.Zipkin.Implementation;
+using OpenTelemetry.Trace;
 using Xunit;
 
 namespace OpenTelemetry.Exporter.Zipkin.Tests.Implementation
@@ -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;
+            }
         }
     }
 }
diff --git a/...nTelemetry.Exporter.Zipkin.Tests/Implementation/ZipkinTraceExporterRemoteEndpointTests.cs b/...nTelemetry.Exporter.Zipkin.Tests/Implementation/ZipkinTraceExporterRemoteEndpointTests.cs