Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Listen to SignalR server activities by default in ASP.NET Core #2429

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+86 −0
Open

Listen to SignalR server activities by default in ASP.NET Core #2429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
30bca7d
Listen to SignalR server activities by default in ASP.NET Core
BrennanConroy Dec 18, 2024
2d10f59
format
BrennanConroy Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions build/Common.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
Refer to https://docs.microsoft.com/en-us/nuget/concepts/package-versioning for semver syntax.
-->
<MinVerPkgVer>[6.0.0,7.0)</MinVerPkgVer>
<MicrosoftAspNetCoreSignalRClientPkgVer>[9.0.0,)</MicrosoftAspNetCoreSignalRClientPkgVer>
<MicrosoftExtensionsHostingAbstractionsPkgVer>[2.1.0,5.0)</MicrosoftExtensionsHostingAbstractionsPkgVer>
<MicrosoftExtensionsConfigurationPkgVer>[9.0.0,)</MicrosoftExtensionsConfigurationPkgVer>
<MicrosoftExtensionsOptionsPkgVer>[9.0.0,)</MicrosoftExtensionsOptionsPkgVer>
Expand Down
7 changes: 7 additions & 0 deletions ...ry.Instrumentation.AspNetCore/AspNetCoreInstrumentationTracerProviderBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,5 +121,12 @@ private static void AddAspNetCoreInstrumentationSources(
builder.AddSource(HttpInListener.ActivitySourceName);
builder.AddLegacySource(HttpInListener.ActivityOperationName); // for the activities created by AspNetCore
}

// SignalR activities first added in .NET 9.0
if (Environment.Version.Major >= 9)
{
// https://github.com/dotnet/aspnetcore/blob/6ae3ea387b20f6497b82897d613e9b8a6e31d69c/src/SignalR/server/Core/src/Internal/SignalRServerActivitySource.cs#L13C35-L13C70
builder.AddSource("Microsoft.AspNetCore.SignalR.Server");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these long (as in, really long) running server spans? I recall ApplicationInsights users complained about recording such long running spans, as that'd skew the latency calculations, and ended up ignoring those spans by default...

dotnet/aspnetcore#32084

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is the case, then we should not do this by default, as there is no API to "remove" a source once added to the provider....

Copy link
Author

@BrennanConroy BrennanConroy Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only long running span should be the http one that has the long running tag. All of the newly added SignalR spans are only for a specific operation.
{4852D1D1-3D32-4C73-8B65-823B1B13C9B6}

Most of the spans added by SignalR add the long running span as a link, but individually they are short lived operations.
{84468919-B48C-4E8C-9E4B-4793E39D5756}
The OnConnectedAsync and OnDisconnectedAsync spans are the only 2 that are parented under the long running span, but they are both very short lived spans themselves.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few years back, I collaborated with your team to introduce the http.long_running attribute in SignalR to help filter long-running HTTP requests in Application Insights .NET SDK. This effort was driven by significant customer feedback about long-running SignalR requests distorting request duration metrics.

With http.long_running part of the default ASP.NET Core instrumentation, customers might face similar issues if long-running spans are automatically included without a suppression mechanism. I recommend considering an option to filter these spans by default or providing a toggle to enable them explicitly before rolling this out.

}
}
}
58 changes: 58 additions & 0 deletions test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc.Testing;
using Microsoft.AspNetCore.SignalR.Client;
using Microsoft.AspNetCore.TestHost;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
Expand Down Expand Up @@ -1114,6 +1115,63 @@ public async Task ValidateUrlQueryRedaction(string urlQuery, string expectedUrlQ
Assert.Equal(expectedUrlQuery, activity.GetTagValue(SemanticConventions.AttributeUrlQuery));
}

#if NET9_0_OR_GREATER
[Fact]
public async Task SignalRActivitesAreListenedTo()
{
var exportedItems = new List<Activity>();
void ConfigureTestServices(IServiceCollection services)
{
this.tracerProvider = Sdk.CreateTracerProviderBuilder()
.AddAspNetCoreInstrumentation()
.AddInMemoryExporter(exportedItems)
.Build();
}

// Arrange
using (var server = this.factory
.WithWebHostBuilder(builder =>
{
builder.ConfigureTestServices(ConfigureTestServices);
builder.ConfigureLogging(loggingBuilder => loggingBuilder.ClearProviders());
}))
{
await using var client = new HubConnectionBuilder()
.WithUrl(server.Server.BaseAddress + "testHub", o =>
{
o.HttpMessageHandlerFactory = _ => server.Server.CreateHandler();
o.Transports = Microsoft.AspNetCore.Http.Connections.HttpTransportType.LongPolling;
}).Build();
await client.StartAsync();

await client.SendAsync("Send", "text");

await client.StopAsync();
}

WaitForActivityExport(exportedItems, 11);

var hubActivity = exportedItems
.Where(a => a.DisplayName.StartsWith("TestApp.AspNetCore.TestHub", StringComparison.InvariantCulture));

Assert.Equal(3, hubActivity.Count());
Assert.Collection(
hubActivity,
one =>
{
Assert.Equal("TestApp.AspNetCore.TestHub/OnConnectedAsync", one.DisplayName);
},
two =>
{
Assert.Equal("TestApp.AspNetCore.TestHub/Send", two.DisplayName);
},
three =>
{
Assert.Equal("TestApp.AspNetCore.TestHub/OnDisconnectedAsync", three.DisplayName);
});
}
#endif

public void Dispose()
{
this.tracerProvider?.Dispose();
Expand Down
1 change: 1 addition & 0 deletions ...ry.Instrumentation.AspNetCore.Tests/OpenTelemetry.Instrumentation.AspNetCore.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
<ItemGroup>
<PackageReference Include="OpenTelemetry.Exporter.InMemory" Version="$(OpenTelemetryCoreLatestVersion)" />
<PackageReference Include="OpenTelemetry.Extensions.Hosting" Version="$(OpenTelemetryCoreLatestVersion)" />
<PackageReference Include="Microsoft.AspNetCore.SignalR.Client" Version="$(MicrosoftAspNetCoreSignalRClientPkgVer)" />
</ItemGroup>

<ItemGroup>
Expand Down
4 changes: 4 additions & 0 deletions test/TestApp.AspNetCore/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ public static void Main(string[] args)

builder.Services.AddMvc();

builder.Services.AddSignalR();

builder.Services.AddSingleton<HttpClient>();

builder.Services.AddSingleton(
Expand All @@ -43,6 +45,8 @@ public static void Main(string[] args)

app.MapControllers();

app.MapHub<TestHub>("/testHub");

app.UseMiddleware<CallbackMiddleware>();

app.UseMiddleware<ActivityMiddleware>();
Expand Down
15 changes: 15 additions & 0 deletions test/TestApp.AspNetCore/TestHub.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

using Microsoft.AspNetCore.SignalR;

namespace TestApp.AspNetCore;

public class TestHub : Hub
{
public override Task OnConnectedAsync() => base.OnConnectedAsync();

public void Send(string message)
{
}
}