diff --git a/src/OpenTelemetry.Exporter.Zipkin/CHANGELOG.md b/src/OpenTelemetry.Exporter.Zipkin/CHANGELOG.md index a1c959756..bf2592860 100644 --- 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 index 6dec36191..ccabc0702 100644 --- a/src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs +++ b/src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs @@ -33,18 +33,21 @@ namespace OpenTelemetry.Exporter.Zipkin.Implementation private static readonly Dictionary RemoteEndpointServiceNameKeyResolutionDictionary = new Dictionary(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 LocalEndpointCache = new ConcurrentDictionary(); + +#if !NET452 + private static readonly ConcurrentDictionary<(string, int), ZipkinEndpoint> RemoteEndpointCache = new ConcurrentDictionary<(string, int), ZipkinEndpoint>(); +#else private static readonly ConcurrentDictionary RemoteEndpointCache = new ConcurrentDictionary(); +#endif private static readonly DictionaryEnumerator.ForEachDelegate ProcessTagsRef = ProcessTags; private static readonly ListEnumerator>.ForEachDelegate ProcessActivityEventsRef = ProcessActivityEvents; @@ -96,9 +99,32 @@ namespace OpenTelemetry.Exporter.Zipkin.Implementation } 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.Create(); @@ -162,6 +188,18 @@ namespace OpenTelemetry.Exporter.Zipkin.Implementation 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 @@ namespace OpenTelemetry.Exporter.Zipkin.Implementation PooledList>.Add(ref state.Tags, new KeyValuePair(key, strVal)); } } + else if (attribute.Value is int intVal && attribute.Key == SemanticConventions.AttributeNetPeerPort) + { + state.Port = intVal; + } else { PooledList>.Add(ref state.Tags, attribute); @@ -224,6 +266,12 @@ namespace OpenTelemetry.Exporter.Zipkin.Implementation 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 index 35d81c16e..6f62be209 100644 --- a/src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinEndpoint.cs +++ b/src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinEndpoint.cs @@ -53,6 +53,17 @@ namespace OpenTelemetry.Exporter.Zipkin.Implementation 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/test/OpenTelemetry.Exporter.Zipkin.Tests/Implementation/ZipkinActivityExporterRemoteEndpointTests.cs b/test/OpenTelemetry.Exporter.Zipkin.Tests/Implementation/ZipkinActivityExporterRemoteEndpointTests.cs index 37a62ed78..973eb15aa 100644 --- a/test/OpenTelemetry.Exporter.Zipkin.Tests/Implementation/ZipkinActivityExporterRemoteEndpointTests.cs +++ b/test/OpenTelemetry.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 @@ namespace OpenTelemetry.Exporter.Zipkin.Tests.Implementation 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 - { - ["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 RemoteEndpointAttributes { get; set; } + + public static IEnumerable GetTestCases() + { + yield return new object[] + { + new RemoteEndpointPriorityTestCase + { + Name = "Highest priority name = net.peer.name", + ExpectedResult = "RemoteServiceName", + RemoteEndpointAttributes = new Dictionary + { + ["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 + { + [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 + { + ["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 + { + ["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 + { + ["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 + { + ["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 + { + ["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/test/OpenTelemetry.Exporter.Zipkin.Tests/Implementation/ZipkinTraceExporterRemoteEndpointTests.cs b/test/OpenTelemetry.Exporter.Zipkin.Tests/Implementation/ZipkinTraceExporterRemoteEndpointTests.cs deleted file mode 100644 index 108c9d303..000000000 --- a/test/OpenTelemetry.Exporter.Zipkin.Tests/Implementation/ZipkinTraceExporterRemoteEndpointTests.cs +++ /dev/null @@ -1,74 +0,0 @@ -// -// 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. -// -using System.Collections.Generic; -using OpenTelemetry.Exporter.Zipkin.Implementation; -using Xunit; - -namespace OpenTelemetry.Exporter.Zipkin.Tests.Implementation -{ - public class ZipkinTraceExporterRemoteEndpointTests - { - private static readonly ZipkinEndpoint DefaultZipkinEndpoint = new ZipkinEndpoint("TestService"); - - [Fact] - public void ZipkinSpanConverterTest_GenerateSpan_RemoteEndpointOmittedByDefault() - { - // Arrange - var activity = ZipkinExporterTests.CreateTestActivity(); - - // Act & Assert - var zipkinSpan = ZipkinActivityConversionExtensions.ToZipkinSpan(activity, DefaultZipkinEndpoint); - - Assert.Null(zipkinSpan.RemoteEndpoint); - } - - [Fact] - public void ZipkinSpanConverterTest_GenerateSpan_RemoteEndpointResolution() - { - // Arrange - var activity = ZipkinExporterTests.CreateTestActivity( - additionalAttributes: new Dictionary - { - ["net.peer.name"] = "RemoteServiceName", - }); - - // Act & Assert - var zipkinSpan = ZipkinActivityConversionExtensions.ToZipkinSpan(activity, DefaultZipkinEndpoint); - - Assert.NotNull(zipkinSpan.RemoteEndpoint); - Assert.Equal("RemoteServiceName", zipkinSpan.RemoteEndpoint.ServiceName); - } - - [Fact] - public void ZipkinSpanConverterTest_GenerateSpan_RemoteEndpointResolutionPriority() - { - // Arrange - var activity = ZipkinExporterTests.CreateTestActivity( - additionalAttributes: new Dictionary - { - ["http.host"] = "DiscardedRemoteServiceName", - ["net.peer.name"] = "RemoteServiceName", - ["peer.hostname"] = "DiscardedRemoteServiceName", - }); - - // Act & Assert - var zipkinSpan = ZipkinActivityConversionExtensions.ToZipkinSpan(activity, DefaultZipkinEndpoint); - - Assert.NotNull(zipkinSpan.RemoteEndpoint); - Assert.Equal("RemoteServiceName", zipkinSpan.RemoteEndpoint.ServiceName); - } - } -}