diff --git a/src/Aspire.Dashboard/Otlp/Model/OtlpSpan.cs b/src/Aspire.Dashboard/Otlp/Model/OtlpSpan.cs index 4caedd73a2c..c6978d288b2 100644 --- a/src/Aspire.Dashboard/Otlp/Model/OtlpSpan.cs +++ b/src/Aspire.Dashboard/Otlp/Model/OtlpSpan.cs @@ -42,7 +42,20 @@ public class OtlpSpan public TimeSpan Duration => EndTime - StartTime; public IEnumerable GetChildSpans() => Trace.Spans.Where(s => s.ParentSpanId == SpanId); - public OtlpSpan? GetParentSpan() => string.IsNullOrEmpty(ParentSpanId) ? null : Trace.Spans.Where(s => s.SpanId == ParentSpanId).FirstOrDefault(); + public OtlpSpan? GetParentSpan() + { + if (string.IsNullOrEmpty(ParentSpanId)) + { + return null; + } + + if (Trace.Spans.TryGetValue(ParentSpanId, out var span)) + { + return span; + } + + return null; + } public OtlpSpan(OtlpApplicationView applicationView, OtlpTrace trace, OtlpScope scope) { diff --git a/src/Aspire.Dashboard/Otlp/Model/OtlpSpanCollection.cs b/src/Aspire.Dashboard/Otlp/Model/OtlpSpanCollection.cs new file mode 100644 index 00000000000..c25cf522d9d --- /dev/null +++ b/src/Aspire.Dashboard/Otlp/Model/OtlpSpanCollection.cs @@ -0,0 +1,27 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.ObjectModel; + +namespace Aspire.Dashboard.Otlp.Model; + +public sealed class OtlpSpanCollection : KeyedCollection +{ + // Chosen to balance memory usage overhead of dictionary vs work to find spans by ID. + private const int DictionaryCreationThreshold = 128; + + public OtlpSpanCollection() : base(StringComparers.OtlpSpanId, DictionaryCreationThreshold) + { + } + + protected override string GetKeyForItem(OtlpSpan item) + { + return item.SpanId; + } + + public new List.Enumerator GetEnumerator() + { + // Avoid allocating an enumerator when iterated with foreach. + return ((List)this.Items).GetEnumerator(); + } +} diff --git a/src/Aspire.Dashboard/Otlp/Model/OtlpTrace.cs b/src/Aspire.Dashboard/Otlp/Model/OtlpTrace.cs index c8a4e6b28f2..aa2cb10d125 100644 --- a/src/Aspire.Dashboard/Otlp/Model/OtlpTrace.cs +++ b/src/Aspire.Dashboard/Otlp/Model/OtlpTrace.cs @@ -34,7 +34,7 @@ public TimeSpan Duration } } - public List Spans { get; } = new List(); + public OtlpSpanCollection Spans { get; } = new OtlpSpanCollection(); public int CalculateDepth(OtlpSpan span) { @@ -52,6 +52,11 @@ public int CalculateDepth(OtlpSpan span) public void AddSpan(OtlpSpan span) { + if (Spans.Contains(span.SpanId)) + { + throw new InvalidOperationException($"Duplicate span id '{span.SpanId}' detected."); + } + var added = false; for (var i = Spans.Count - 1; i >= 0; i--) { @@ -65,12 +70,12 @@ public void AddSpan(OtlpSpan span) if (!added) { Spans.Insert(0, span); + } - // If there isn't a root span then the first span is used as the trace name. - if (_rootSpan == null && !string.IsNullOrEmpty(span.ParentSpanId)) - { - FullName = BuildFullName(span); - } + if (HasCircularReference(span)) + { + Spans.Remove(span); + throw new InvalidOperationException($"Circular loop detected for span '{span.SpanId}' with parent '{span.ParentSpanId}'."); } if (string.IsNullOrEmpty(span.ParentSpanId)) @@ -87,6 +92,11 @@ public void AddSpan(OtlpSpan span) } } } + else if (_rootSpan == null && span == Spans[0]) + { + // If there isn't a root span then the first span is used as the trace name. + FullName = BuildFullName(span); + } AssertSpanOrder(); @@ -96,6 +106,31 @@ static string BuildFullName(OtlpSpan existingSpan) } } + private static bool HasCircularReference(OtlpSpan span) + { + // Can't have a circular reference if the span has no parent. + if (string.IsNullOrEmpty(span.ParentSpanId)) + { + return false; + } + + // Walk up span ancestors to check there is no loop. + var stack = new OtlpSpanCollection { span }; + var currentSpan = span; + while (currentSpan.GetParentSpan() is { } parentSpan) + { + if (stack.Contains(parentSpan)) + { + return true; + } + + stack.Add(parentSpan); + currentSpan = parentSpan; + } + + return false; + } + [Conditional("DEBUG")] private void AssertSpanOrder() { diff --git a/src/Aspire.Dashboard/Otlp/Model/OtlpTraceCollection.cs b/src/Aspire.Dashboard/Otlp/Model/OtlpTraceCollection.cs deleted file mode 100644 index 0c214e8aa72..00000000000 --- a/src/Aspire.Dashboard/Otlp/Model/OtlpTraceCollection.cs +++ /dev/null @@ -1,42 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System.Collections.ObjectModel; - -namespace Aspire.Dashboard.Otlp.Model; - -public sealed class OtlpTraceCollection : KeyedCollection, OtlpTrace> -{ - public OtlpTraceCollection() : base(MemoryComparable.Instance, dictionaryCreationThreshold: 0) - { - - } - - protected override ReadOnlyMemory GetKeyForItem(OtlpTrace item) - { - return item.Key; - } - - private sealed class MemoryComparable : IEqualityComparer> - { - public static readonly MemoryComparable Instance = new(); - - public bool Equals(ReadOnlyMemory x, ReadOnlyMemory y) - { - return x.Span.SequenceEqual(y.Span); - } - - public int GetHashCode(ReadOnlyMemory obj) - { - unchecked - { - var hash = 17; - foreach (var value in obj.Span) - { - hash = hash * 23 + value.GetHashCode(); - } - return hash; - } - } - } -} diff --git a/src/Shared/StringComparers.cs b/src/Shared/StringComparers.cs index d36e94421be..b35809f8065 100644 --- a/src/Shared/StringComparers.cs +++ b/src/Shared/StringComparers.cs @@ -23,6 +23,7 @@ internal static class StringComparers public static StringComparer GridColumn => StringComparer.Ordinal; public static StringComparer OtlpAttribute => StringComparer.Ordinal; public static StringComparer OtlpFieldValue => StringComparer.OrdinalIgnoreCase; + public static StringComparer OtlpSpanId => StringComparer.Ordinal; } internal static class StringComparisons @@ -43,4 +44,5 @@ internal static class StringComparisons public static StringComparison GridColumn => StringComparison.Ordinal; public static StringComparison OtlpAttribute => StringComparison.Ordinal; public static StringComparison OtlpFieldValue => StringComparison.OrdinalIgnoreCase; + public static StringComparison OtlpSpanId => StringComparison.Ordinal; } diff --git a/tests/Aspire.Dashboard.Tests/TelemetryRepositoryTests/TraceTests.cs b/tests/Aspire.Dashboard.Tests/TelemetryRepositoryTests/TraceTests.cs index 87beb26abb9..5091787478f 100644 --- a/tests/Aspire.Dashboard.Tests/TelemetryRepositoryTests/TraceTests.cs +++ b/tests/Aspire.Dashboard.Tests/TelemetryRepositoryTests/TraceTests.cs @@ -8,6 +8,8 @@ using Aspire.Dashboard.Otlp.Storage; using Google.Protobuf; using Google.Protobuf.Collections; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Testing; using OpenTelemetry.Proto.Common.V1; using OpenTelemetry.Proto.Trace.V1; using Xunit; @@ -90,6 +92,172 @@ public void AddTraces() }); } + [Fact] + public void AddTraces_SelfParent_Reject() + { + // Arrange + var testSink = new TestSink(); + var factory = LoggerFactory.Create(b => b.AddProvider(new TestLoggerProvider(testSink))); + + var repository = CreateRepository(loggerFactory: factory); + + // Act + var addContext = new AddContext(); + repository.AddTraces(addContext, new RepeatedField() + { + new ResourceSpans + { + Resource = CreateResource(), + ScopeSpans = + { + new ScopeSpans + { + Scope = CreateScope(), + Spans = + { + CreateSpan(traceId: "1", spanId: "1-1", startTime: s_testTime.AddMinutes(1), endTime: s_testTime.AddMinutes(10), parentSpanId: "1-1") + } + } + } + } + }); + + // Assert + Assert.Equal(1, addContext.FailureCount); + + var applications = repository.GetApplications(); + Assert.Collection(applications, + app => + { + Assert.Equal("TestService", app.ApplicationName); + Assert.Equal("TestId", app.InstanceId); + }); + + var traces = repository.GetTraces(new GetTracesRequest + { + ApplicationKey = applications[0].ApplicationKey, + FilterText = string.Empty, + StartIndex = 0, + Count = 10, + Filters = [] + }); + Assert.Empty(traces.PagedResult.Items); + + var write = Assert.Single(testSink.Writes); + Assert.Equal("Error adding span.", write.Message); + Assert.Equal("Circular loop detected for span '312d31' with parent '312d31'.", write.Exception!.Message); + } + + [Fact] + public void AddTraces_MultipleSpansLoop_Reject() + { + // Arrange + var repository = CreateRepository(); + + // Act + var addContext = new AddContext(); + repository.AddTraces(addContext, new RepeatedField() + { + new ResourceSpans + { + Resource = CreateResource(), + ScopeSpans = + { + new ScopeSpans + { + Scope = CreateScope(), + Spans = + { + CreateSpan(traceId: "1", spanId: "1-1", startTime: s_testTime.AddMinutes(1), endTime: s_testTime.AddMinutes(10), parentSpanId: "1-3"), + CreateSpan(traceId: "1", spanId: "1-2", startTime: s_testTime.AddMinutes(5), endTime: s_testTime.AddMinutes(10), parentSpanId: "1-1"), + CreateSpan(traceId: "1", spanId: "1-3", startTime: s_testTime.AddMinutes(5), endTime: s_testTime.AddMinutes(10), parentSpanId: "1-2") + } + } + } + } + }); + + // Assert + Assert.Equal(1, addContext.FailureCount); + + var applications = repository.GetApplications(); + Assert.Collection(applications, + app => + { + Assert.Equal("TestService", app.ApplicationName); + Assert.Equal("TestId", app.InstanceId); + }); + + var traces = repository.GetTraces(new GetTracesRequest + { + ApplicationKey = applications[0].ApplicationKey, + FilterText = string.Empty, + StartIndex = 0, + Count = 10, + Filters = [] + }); + Assert.Collection(traces.PagedResult.Items, + trace => + { + Assert.Equal(2, trace.Spans.Count); + }); + } + + [Fact] + public void AddTraces_DuplicateTraceIds_Reject() + { + // Arrange + var repository = CreateRepository(); + + // Act + var addContext = new AddContext(); + repository.AddTraces(addContext, new RepeatedField() + { + new ResourceSpans + { + Resource = CreateResource(), + ScopeSpans = + { + new ScopeSpans + { + Scope = CreateScope(), + Spans = + { + CreateSpan(traceId: "1", spanId: "1-1", startTime: s_testTime.AddMinutes(1), endTime: s_testTime.AddMinutes(10)), + CreateSpan(traceId: "1", spanId: "1-1", startTime: s_testTime.AddMinutes(5), endTime: s_testTime.AddMinutes(10)), + CreateSpan(traceId: "1", spanId: "1-2", startTime: s_testTime.AddMinutes(1), endTime: s_testTime.AddMinutes(10), parentSpanId: "1-1"), + } + } + } + } + }); + + // Assert + Assert.Equal(1, addContext.FailureCount); + + var applications = repository.GetApplications(); + Assert.Collection(applications, + app => + { + Assert.Equal("TestService", app.ApplicationName); + Assert.Equal("TestId", app.InstanceId); + }); + + var traces = repository.GetTraces(new GetTracesRequest + { + ApplicationKey = applications[0].ApplicationKey, + FilterText = string.Empty, + StartIndex = 0, + Count = 10, + Filters = [] + }); + Assert.Collection(traces.PagedResult.Items, + trace => + { + Assert.Equal(2, trace.Spans.Count); + }); + } + [Fact] public void AddTraces_Scope_Multiple() {