Skip to content

Commit 3142278

Browse files
committed
Address review feedback: skip channel allocation when no filter, remove Task.Delay from tests
1 parent 5c5f36d commit 3142278

2 files changed

Lines changed: 38 additions & 52 deletions

File tree

src/Aspire.Dashboard/Api/TelemetryApiService.cs

Lines changed: 22 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -46,9 +46,9 @@ internal sealed class TelemetryApiService(
4646
var spanFilters = new List<TelemetryFilter>();
4747
var searchTextFragments = ParseAndApplySearchFilters(search, spanFilters, AddSpanFiltersFromQualifiers, key => ResolveSpanFieldKey(key) is not null);
4848

49-
// Get spans for all resource keys
49+
// Get spans for all resource keys (empty list means no filter / all resources)
5050
var allSpans = new List<OtlpSpan>();
51-
foreach (var resourceKey in resourceKeys)
51+
foreach (var resourceKey in resourceKeys.Count > 0 ? resourceKeys.Select(k => (ResourceKey?)k) : [null])
5252
{
5353
var result = telemetryRepository.GetSpans(new GetSpansRequest
5454
{
@@ -103,9 +103,9 @@ internal sealed class TelemetryApiService(
103103
var traceFilters = new List<TelemetryFilter>();
104104
var searchTextFragments = ParseAndApplySearchFilters(search, traceFilters, AddSpanFiltersFromQualifiers, key => ResolveSpanFieldKey(key) is not null);
105105

106-
// Get traces for all resource keys
106+
// Get traces for all resource keys (empty list means no filter / all resources)
107107
var allTraces = new List<OtlpTrace>();
108-
foreach (var resourceKey in resourceKeys)
108+
foreach (var resourceKey in resourceKeys.Count > 0 ? resourceKeys.Select(k => (ResourceKey?)k) : [null])
109109
{
110110
var result = telemetryRepository.GetTraces(new GetTracesRequest
111111
{
@@ -222,8 +222,9 @@ internal sealed class TelemetryApiService(
222222
var searchTextFragments = ParseAndApplySearchFilters(search, filters, AddLogFiltersFromQualifiers, key => ResolveLogFieldKey(key) is not null);
223223

224224
// Get logs for all resource keys
225+
// Get logs for all resource keys (empty list means no filter / all resources)
225226
var allLogs = new List<OtlpLogEntry>();
226-
foreach (var resourceKey in resourceKeys)
227+
foreach (var resourceKey in resourceKeys.Count > 0 ? resourceKeys.Select(k => (ResourceKey?)k) : [null])
227228
{
228229
var result = telemetryRepository.GetLogs(new GetLogsContext
229230
{
@@ -278,7 +279,7 @@ public async IAsyncEnumerable<string> FollowSpansAsync(
278279
// Build the watch request with all filters pushed into the repository
279280
var watchRequest = new WatchSpansRequest
280281
{
281-
ResourceKey = resourceKeys is { Count: 1 } ? resourceKeys[0] : null,
282+
ResourceKey = resourceKeys is [var singleKey] ? singleKey : null,
282283
Filters = spanFilters,
283284
TraceId = traceId,
284285
HasError = hasError,
@@ -291,7 +292,7 @@ public async IAsyncEnumerable<string> FollowSpansAsync(
291292
// Multi-resource filtering: repository only supports single ResourceKey,
292293
// so for multi-resource queries we filter here.
293294
if (resourceKeys is { Count: > 1 } &&
294-
!resourceKeys.Any(k => k?.EqualsCompositeName(span.Source.ResourceKey.GetCompositeName()) == true))
295+
!resourceKeys.Any(k => k.EqualsCompositeName(span.Source.ResourceKey.GetCompositeName())))
295296
{
296297
continue;
297298
}
@@ -348,7 +349,7 @@ public async IAsyncEnumerable<string> FollowLogsAsync(
348349
// Build the watch request with all filters pushed into the repository
349350
var watchRequest = new WatchLogsRequest
350351
{
351-
ResourceKey = resourceKeys is { Count: 1 } ? resourceKeys[0] : null,
352+
ResourceKey = resourceKeys is [var singleKey] ? singleKey : null,
352353
Filters = filters,
353354
TextFragments = searchTextFragments
354355
};
@@ -359,7 +360,7 @@ public async IAsyncEnumerable<string> FollowLogsAsync(
359360
// Multi-resource filtering: repository only supports single ResourceKey,
360361
// so for multi-resource queries we filter here.
361362
if (resourceKeys is { Count: > 1 } &&
362-
!resourceKeys.Any(k => k?.EqualsCompositeName(log.ResourceView.ResourceKey.GetCompositeName()) == true))
363+
!resourceKeys.Any(k => k.EqualsCompositeName(log.ResourceView.ResourceKey.GetCompositeName())))
363364
{
364365
continue;
365366
}
@@ -551,8 +552,14 @@ private static void AddSpanFiltersFromQualifiers(SearchFilter parsedSearch, List
551552
/// to pick up data once the resource is first seen.
552553
/// Throws OperationCanceledException if cancellation is triggered before the resources appear.
553554
/// </summary>
554-
private async Task<List<ResourceKey?>> WaitForResourceKeysAsync(string[]? resourceNames, CancellationToken cancellationToken)
555+
private async Task<List<ResourceKey>> WaitForResourceKeysAsync(string[]? resourceNames, CancellationToken cancellationToken)
555556
{
557+
if (resourceNames is null || resourceNames.Length == 0)
558+
{
559+
// No filter - return immediately without allocating a channel or subscription.
560+
return [];
561+
}
562+
556563
// Subscribe before the first check so no notification can be missed between
557564
// GetResources() and the subscription registration.
558565
var signal = Channel.CreateBounded<bool>(new BoundedChannelOptions(1) { FullMode = BoundedChannelFullMode.DropOldest });
@@ -577,24 +584,23 @@ private static void AddSpanFiltersFromQualifiers(SearchFilter parsedSearch, List
577584
/// <summary>
578585
/// Resolves resource names to ResourceKeys.
579586
/// Returns null if any specified resource is not found.
580-
/// If no resources are specified, returns a list with a single null key (no filter).
587+
/// Returns an empty list when no resource filter is specified (meaning all resources).
581588
/// </summary>
582-
private static List<ResourceKey?>? ResolveResourceKeys(IReadOnlyList<OtlpResource> resources, string[]? resourceNames)
589+
private static List<ResourceKey>? ResolveResourceKeys(IReadOnlyList<OtlpResource> resources, string[]? resourceNames)
583590
{
584591
if (resourceNames is null || resourceNames.Length == 0)
585592
{
586-
// No filter - return a list with null to indicate "all resources"
587-
return [null];
593+
return [];
588594
}
589595

590-
var keys = new List<ResourceKey?>();
596+
var keys = new List<ResourceKey>();
591597
foreach (var resourceName in resourceNames)
592598
{
593599
if (!AIHelpers.TryResolveResourceForTelemetry(resources, resourceName, out _, out var resourceKey))
594600
{
595601
return null;
596602
}
597-
keys.Add(resourceKey);
603+
keys.Add(resourceKey!.Value);
598604
}
599605
return keys;
600606
}

tests/Aspire.Dashboard.Tests/TelemetryApiServiceTests.cs

Lines changed: 16 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -652,30 +652,20 @@ public async Task FollowSpansAsync_WaitsForResourceToAppear_ThenStreams()
652652
var service = CreateService(repository);
653653
using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(10));
654654

655-
var receivedItems = new List<string>();
656-
657-
// Start streaming for a resource that doesn't exist yet.
658-
var streamTask = Task.Run(async () =>
659-
{
660-
await foreach (var item in service.FollowSpansAsync(["service1"], null, null, null, cancellationToken: cts.Token))
661-
{
662-
receivedItems.Add(item);
663-
if (receivedItems.Count >= 1)
664-
{
665-
break;
666-
}
667-
}
668-
}, cts.Token);
655+
// Start enumerating - MoveNextAsync will block until data arrives.
656+
var enumerator = service.FollowSpansAsync(["service1"], null, null, null, cancellationToken: cts.Token).GetAsyncEnumerator(cts.Token);
657+
var moveNextTask = enumerator.MoveNextAsync();
669658

670-
// Give the streaming task time to start waiting for the resource.
671-
await Task.Delay(100, cts.Token);
659+
// The task should not complete yet because the resource doesn't exist.
660+
Assert.False(moveNextTask.IsCompleted);
672661

673662
// Now add spans for the resource - this should unblock the stream.
674663
AddSpans(repository, count: 1);
675664

676-
await streamTask;
665+
Assert.True(await moveNextTask);
666+
Assert.NotNull(enumerator.Current);
677667

678-
Assert.Single(receivedItems);
668+
await enumerator.DisposeAsync();
679669
}
680670

681671
[Fact]
@@ -685,30 +675,20 @@ public async Task FollowLogsAsync_WaitsForResourceToAppear_ThenStreams()
685675
var service = CreateService(repository);
686676
using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(10));
687677

688-
var receivedItems = new List<string>();
689-
690-
// Start streaming for a resource that doesn't exist yet.
691-
var streamTask = Task.Run(async () =>
692-
{
693-
await foreach (var item in service.FollowLogsAsync(["service1"], null, null, null, cts.Token))
694-
{
695-
receivedItems.Add(item);
696-
if (receivedItems.Count >= 1)
697-
{
698-
break;
699-
}
700-
}
701-
}, cts.Token);
678+
// Start enumerating - MoveNextAsync will block until data arrives.
679+
var enumerator = service.FollowLogsAsync(["service1"], null, null, null, cts.Token).GetAsyncEnumerator(cts.Token);
680+
var moveNextTask = enumerator.MoveNextAsync();
702681

703-
// Give the streaming task time to start waiting for the resource.
704-
await Task.Delay(100, cts.Token);
682+
// The task should not complete yet because the resource doesn't exist.
683+
Assert.False(moveNextTask.IsCompleted);
705684

706685
// Now add logs for the resource - this should unblock the stream.
707686
AddLogs(repository, ["hello"]);
708687

709-
await streamTask;
688+
Assert.True(await moveNextTask);
689+
Assert.NotNull(enumerator.Current);
710690

711-
Assert.Single(receivedItems);
691+
await enumerator.DisposeAsync();
712692
}
713693

714694
// SpanId is serialized as lowercase hex per the OTLP/JSON spec

0 commit comments

Comments
 (0)