From 19f834fc82f208af734415f9b965a525845db208 Mon Sep 17 00:00:00 2001 From: Alan West <3676547+alanwest@users.noreply.github.com> Date: Mon, 31 Aug 2020 20:09:58 -0700 Subject: [PATCH] Jaeger: Include net.peer.port in peer.service attribute (#1195) * Include net.peer.port in peer.service attribute when applicable * Update changelog * Small change to if statement Co-authored-by: Cijo Thomas --- .../CHANGELOG.md | 3 + .../JaegerActivityExtensions.cs | 72 +++++++-- .../JaegerActivityConversionTest.cs | 145 ++++++++++++++++-- 3 files changed, 192 insertions(+), 28 deletions(-) diff --git a/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md b/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md index c841d8093..218c0cdaa 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md +++ b/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md @@ -11,6 +11,9 @@ Released 2020-08-28 * Span links will now be sent as `FOLLOWS_FROM` reference type. Previously they were sent as `CHILD_OF`. ([#970](https://github.com/open-telemetry/opentelemetry-dotnet/pull/970)) +* Fixed issue when span has both the `net.peer.name` and `net.peer.port` + attributes but did not include `net.peer.port` in the `peer.service` field + ([#1195](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1195)). * Renamed extension method from `UseJaegerExporter` to `AddJaegerExporter`. diff --git a/src/OpenTelemetry.Exporter.Jaeger/Implementation/JaegerActivityExtensions.cs b/src/OpenTelemetry.Exporter.Jaeger/Implementation/JaegerActivityExtensions.cs index 46bedfa9d..a5d031b5f 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/Implementation/JaegerActivityExtensions.cs +++ b/src/OpenTelemetry.Exporter.Jaeger/Implementation/JaegerActivityExtensions.cs @@ -44,12 +44,10 @@ namespace OpenTelemetry.Exporter.Jaeger.Implementation private static readonly Dictionary PeerServiceKeyResolutionDictionary = 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, // peer.service for Http. - [SemanticConventions.AttributeDbInstance] = 3, // peer.service for Redis. + ["peer.hostname"] = 1, + ["peer.address"] = 1, + [SemanticConventions.AttributeHttpHost] = 2, // peer.service for Http. + [SemanticConventions.AttributeDbInstance] = 2, // peer.service for Redis. }; private static readonly DictionaryEnumerator.ForEachDelegate ProcessActivityTagRef = ProcessActivityTag; @@ -70,13 +68,31 @@ namespace OpenTelemetry.Exporter.Jaeger.Implementation ProcessActivityTagRef); string peerServiceName = null; - if ((activity.Kind == ActivityKind.Client || activity.Kind == ActivityKind.Producer) && jaegerTags.PeerService != null) + if (activity.Kind == ActivityKind.Client || activity.Kind == ActivityKind.Producer) { - // Send peer.service for remote calls. - peerServiceName = jaegerTags.PeerService; + // If priority = 0 that means peer.service may have already been included in tags + var addPeerServiceTag = jaegerTags.PeerServicePriority > 0; - // If priority = 0 that means peer.service was already included in tags. - if (jaegerTags.PeerServicePriority > 0) + var hostNameOrIpAddress = jaegerTags.HostName ?? jaegerTags.IpAddress; + + // peer.service has not already been included, but net.peer.name/ip and optionally net.peer.port are present + if ((jaegerTags.PeerService == null || addPeerServiceTag) + && hostNameOrIpAddress != null) + { + peerServiceName = jaegerTags.Port == default + ? hostNameOrIpAddress + : $"{hostNameOrIpAddress}:{jaegerTags.Port}"; + + // Add the peer.service tag + addPeerServiceTag = true; + } + + if (peerServiceName == null && jaegerTags.PeerService != null) + { + peerServiceName = jaegerTags.PeerService; + } + + if (peerServiceName != null && addPeerServiceTag) { PooledList.Add(ref jaegerTags.Tags, new JaegerTag(SemanticConventions.AttributePeerService, JaegerTagType.STRING, vStr: peerServiceName)); } @@ -291,12 +307,30 @@ namespace OpenTelemetry.Exporter.Jaeger.Implementation private static void ProcessJaegerTag(ref TagState state, string key, JaegerTag jaegerTag) { - if (jaegerTag.VStr != null - && PeerServiceKeyResolutionDictionary.TryGetValue(key, out int priority) - && (state.PeerService == null || priority < state.PeerServicePriority)) + if (jaegerTag.VStr != null) { - state.PeerService = jaegerTag.VStr; - state.PeerServicePriority = priority; + if (PeerServiceKeyResolutionDictionary.TryGetValue(key, out int priority) + && (state.PeerService == null || priority < state.PeerServicePriority)) + { + state.PeerService = jaegerTag.VStr; + state.PeerServicePriority = priority; + } + else if (key == SemanticConventions.AttributeNetPeerName) + { + state.HostName = jaegerTag.VStr; + } + else if (key == SemanticConventions.AttributeNetPeerIp) + { + state.IpAddress = jaegerTag.VStr; + } + else if (key == SemanticConventions.AttributeNetPeerPort && long.TryParse(jaegerTag.VStr, out var port)) + { + state.Port = port; + } + } + else if (jaegerTag.VLong.HasValue && key == SemanticConventions.AttributeNetPeerPort) + { + state.Port = jaegerTag.VLong.Value; } PooledList.Add(ref state.Tags, jaegerTag); @@ -340,6 +374,12 @@ namespace OpenTelemetry.Exporter.Jaeger.Implementation public string PeerService; public int PeerServicePriority; + + public string HostName; + + public string IpAddress; + + public long Port; } private struct PooledListState diff --git a/test/OpenTelemetry.Exporter.Jaeger.Tests/Implementation/JaegerActivityConversionTest.cs b/test/OpenTelemetry.Exporter.Jaeger.Tests/Implementation/JaegerActivityConversionTest.cs index bfb4cff53..12594daaa 100644 --- a/test/OpenTelemetry.Exporter.Jaeger.Tests/Implementation/JaegerActivityConversionTest.cs +++ b/test/OpenTelemetry.Exporter.Jaeger.Tests/Implementation/JaegerActivityConversionTest.cs @@ -20,7 +20,7 @@ using System.Linq; using OpenTelemetry.Exporter.Jaeger.Implementation; using OpenTelemetry.Resources; - +using OpenTelemetry.Trace; using Xunit; namespace OpenTelemetry.Exporter.Jaeger.Tests.Implementation @@ -396,26 +396,21 @@ namespace OpenTelemetry.Exporter.Jaeger.Tests.Implementation Assert.Empty(jaegerSpan.Tags.Where(t => t.Key == "peer.service")); } - [Fact] - public void JaegerActivityConverterTest_GenerateJaegerSpan_RemoteEndpointResolutionPriority() + [Theory] + [MemberData(nameof(RemoteEndpointPriorityTestCase.GetTestCases), MemberType = typeof(RemoteEndpointPriorityTestCase))] + public void JaegerActivityConverterTest_GenerateJaegerSpan_RemoteEndpointResolutionPriority(RemoteEndpointPriorityTestCase testCase) { // Arrange - var span = CreateTestActivity( - additionalAttributes: new Dictionary - { - ["http.host"] = "DiscardedRemoteServiceName", - ["peer.service"] = "RemoteServiceName", - ["peer.hostname"] = "DiscardedRemoteServiceName", - }); + var activity = CreateTestActivity(additionalAttributes: testCase.RemoteEndpointAttributes); // Act - var jaegerSpan = span.ToJaegerSpan(); + var jaegerSpan = activity.ToJaegerSpan(); // Assert var tags = jaegerSpan.Tags.Where(t => t.Key == "peer.service"); Assert.Single(tags); var tag = tags.First(); - Assert.Equal("RemoteServiceName", tag.VStr); + Assert.Equal(testCase.ExpectedResult, tag.VStr); } internal static Activity CreateTestActivity( @@ -510,5 +505,131 @@ namespace OpenTelemetry.Exporter.Jaeger.Tests.Implementation return activity; } + + 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; + } + } } }