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

Jump to bottom

Update Grpc.Net.Client to use ActivitySource #2244

Merged
merged 6 commits into from
Aug 17, 2023
Merged

Update Grpc.Net.Client to use ActivitySource #2244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3507296
Update Grpc.Net.Client to use ActivitySource
JamesNK Aug 15, 2023
3181bfe
Fix tests
JamesNK Aug 15, 2023
83d9037
Comment
JamesNK Aug 15, 2023
d6858d3
PR feedback
JamesNK Aug 15, 2023
2dd142c
Clean up
JamesNK Aug 16, 2023
de688df
Update src/Grpc.Net.Client/Internal/GrpcCall.cs
JamesNK Aug 16, 2023
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
2 changes: 1 addition & 1 deletion build/dependencies.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
<ProtobufNetGrpcPackageVersion>1.0.140</ProtobufNetGrpcPackageVersion>
<SystemCommandLinePackageVersion>2.0.0-beta4.22272.1</SystemCommandLinePackageVersion>
<SystemCommandLineRenderingPackageVersion>0.4.0-alpha.22272.1</SystemCommandLineRenderingPackageVersion>
<SystemDiagnosticsDiagnosticSourcePackageVersion>4.5.1</SystemDiagnosticsDiagnosticSourcePackageVersion>
<SystemDiagnosticsDiagnosticSourcePackageVersion>6.0.1</SystemDiagnosticsDiagnosticSourcePackageVersion>
<SystemIOPipelinesPackageVersion>5.0.1</SystemIOPipelinesPackageVersion>
<SystemMemoryPackageVersion>4.5.3</SystemMemoryPackageVersion>
<SystemNetHttpWinHttpHandlerPackageVersion>7.0.0</SystemNetHttpWinHttpHandlerPackageVersion>
Expand Down
9 changes: 1 addition & 8 deletions src/Grpc.Net.Client/Balancer/Internal/ConnectionManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#region Copyright notice and license
#region Copyright notice and license

// Copyright 2019 The gRPC Authors
//
Expand All @@ -17,17 +17,10 @@
#endregion

#if SUPPORT_LOAD_BALANCING
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Net;
using System.Threading;
using System.Threading.Tasks;
using Grpc.Core;
using Grpc.Net.Client.Configuration;
using Grpc.Net.Client.Internal;
using Grpc.Shared;
using Microsoft.Extensions.Logging;

namespace Grpc.Net.Client.Balancer.Internal;
Expand Down
8 changes: 0 additions & 8 deletions src/Grpc.Net.Client/Balancer/Internal/SocketConnectivitySubchannelTransport.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,9 @@
#endregion

#if SUPPORT_LOAD_BALANCING
using System;
using System.Buffers;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Net;
using System.Net.Http;
using System.Net.Sockets;
using System.Threading;
using System.Threading.Tasks;
using Grpc.Core;
using Grpc.Shared;
using Microsoft.Extensions.Logging;
Expand Down
6 changes: 0 additions & 6 deletions src/Grpc.Net.Client/Balancer/Internal/StreamWrapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,6 @@
#endregion

#if SUPPORT_LOAD_BALANCING
using System;
using System.Diagnostics;
using System.IO;
using System.Threading;
using System.Threading.Tasks;

namespace Grpc.Net.Client.Balancer.Internal;

internal sealed class StreamWrapper : Stream
Expand Down
5 changes: 1 addition & 4 deletions src/Grpc.Net.Client/Balancer/RoundRobinBalancer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#region Copyright notice and license
#region Copyright notice and license

// Copyright 2019 The gRPC Authors
//
Expand All @@ -17,9 +17,6 @@
#endregion

#if SUPPORT_LOAD_BALANCING
using System.Collections.Generic;
using System.Linq;
using System.Threading;
using Grpc.Net.Client.Configuration;
using Grpc.Net.Client.Internal;
using Microsoft.Extensions.Logging;
Expand Down
7 changes: 0 additions & 7 deletions src/Grpc.Net.Client/Balancer/Subchannel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,6 @@
#endregion

#if SUPPORT_LOAD_BALANCING
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Net;
using System.Threading;
using System.Threading.Tasks;
using Grpc.Core;
using Grpc.Net.Client.Balancer.Internal;
using Microsoft.Extensions.Logging;
Expand Down
7 changes: 1 addition & 6 deletions src/Grpc.Net.Client/Balancer/SubchannelsLoadBalancer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#region Copyright notice and license
#region Copyright notice and license

// Copyright 2019 The gRPC Authors
//
Expand All @@ -17,11 +17,6 @@
#endregion

#if SUPPORT_LOAD_BALANCING
using System;
using System.Collections.Generic;
using System.Globalization;
using System.Linq;
using System.Net;
using Grpc.Core;
using Grpc.Net.Client.Balancer.Internal;
using Microsoft.Extensions.Logging;
Expand Down
1 change: 0 additions & 1 deletion src/Grpc.Net.Client/Grpc.Net.Client.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
<Compile Include="..\Shared\CommonGrpcProtocolHelpers.cs" Link="Internal\CommonGrpcProtocolHelpers.cs" />
<Compile Include="..\Shared\DefaultDeserializationContext.cs" Link="Internal\DefaultDeserializationContext.cs" />
<Compile Include="..\Shared\HttpHandlerFactory.cs" Link="Internal\Http\HttpHandlerFactory.cs" />
<Compile Include="..\Shared\TelemetryHeaderHandler.cs" Link="Internal\Http\TelemetryHeaderHandler.cs" />
<Compile Include="..\Shared\HttpRequestHelpers.cs" Link="Internal\Http\HttpRequestHelpers.cs" />
<Compile Include="..\Shared\TrailingHeadersHelpers.cs" Link="Internal\Http\TrailingHeadersHelpers.cs" />
<Compile Include="..\Shared\CompatibilityHelpers.cs" Link="Internal\CompatibilityHelpers.cs" />
Expand Down
4 changes: 0 additions & 4 deletions src/Grpc.Net.Client/GrpcChannel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,10 +508,6 @@ private HttpMessageInvoker CreateInternalHttpInvoker(HttpMessageHandler? handler
}
}

#if NET5_0
handler = HttpHandlerFactory.EnsureTelemetryHandler(handler);
#endif

#if SUPPORT_LOAD_BALANCING
BalancerHttpHandler balancerHttpHandler;
handler = balancerHttpHandler = new BalancerHttpHandler(handler, ConnectionManager);
Expand Down
10 changes: 8 additions & 2 deletions src/Grpc.Net.Client/Internal/GrpcCall.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -811,9 +811,15 @@ private void SetFailedResult(Status status)
// 1. Diagnostic source is enabled
// 2. Logging is enabled
// 3. There is an existing activity (to enable activity propagation)
if (diagnosticSourceEnabled || Logger.IsEnabled(LogLevel.Critical) || Activity.Current != null)
// 4. ActivitySource has listeners
if (diagnosticSourceEnabled || Logger.IsEnabled(LogLevel.Critical) || Activity.Current != null || GrpcDiagnostics.ActivitySource.HasListeners())
{
activity = new Activity(GrpcDiagnostics.ActivityName);
activity = GrpcDiagnostics.ActivitySource.CreateActivity(GrpcDiagnostics.ActivityName, ActivityKind.Client);

// ActivitySource only returns an activity if someone is listening.
// If we're at this point then we always want there to be an activity. Create the activity manually.
activity ??= new Activity(GrpcDiagnostics.ActivityName);
Copy link
Contributor

Choose a reason for hiding this comment

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

How come this one doesn't have an ActivityKind? Is Client the default?

Copy link
Member Author

Choose a reason for hiding this comment

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

The default is internal, but it can't be set when using the Activity ctor.

Copy link

Choose a reason for hiding this comment

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

We can consider in the future release to add a constructor that can take a ActivityKind. I think this can help in such scenarios but still will not help older Grpc client versions that have already been released.


activity.AddTag(GrpcDiagnostics.GrpcMethodTagName, Method.FullName);
activity.Start();

Expand Down
5 changes: 3 additions & 2 deletions src/Grpc.Net.Client/Internal/GrpcDiagnostics.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#region Copyright notice and license
#region Copyright notice and license

// Copyright 2019 The gRPC Authors
//
Expand All @@ -22,8 +22,9 @@ namespace Grpc.Net.Client.Internal;

internal static class GrpcDiagnostics
{
// This is a static on a non-generic class so it isn't re-created once for each type argument
// These are static on a non-generic class. We don't want these re-created for each type argument.
public static readonly DiagnosticListener DiagnosticListener = new DiagnosticListener("Grpc.Net.Client");
public static readonly ActivitySource ActivitySource = new ActivitySource("Grpc.Net.Client");

public const string ActivityName = "Grpc.Net.Client.GrpcOut";

Expand Down
1 change: 0 additions & 1 deletion src/Grpc.Net.ClientFactory/Grpc.Net.ClientFactory.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@

<Compile Include="..\Shared\IsExternalInit.cs" Link="Internal\IsExternalInit.cs" />
<Compile Include="..\Shared\HttpHandlerFactory.cs" Link="Internal\HttpHandlerFactory.cs" />
<Compile Include="..\Shared\TelemetryHeaderHandler.cs" Link="Internal\TelemetryHeaderHandler.cs" />
<Compile Include="..\Shared\CompatibilityHelpers.cs" Link="Internal\CompatibilityHelpers.cs" />
<Compile Include="..\Shared\NullableAttributes.cs" Link="Internal\NullableAttributes.cs" />
<Compile Include="..\Shared\HttpRequestHelpers.cs" Link="Internal\Http\HttpRequestHelpers.cs" />
Expand Down
3 changes: 0 additions & 3 deletions src/Grpc.Net.ClientFactory/GrpcClientServiceExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,9 +335,6 @@ private static IHttpClientBuilder AddGrpcHttpClient<
socketsHttpHandler.PooledConnectionLifetime = options.HandlerLifetime;
}
#endif
#if NET5_0
handler = HttpHandlerFactory.EnsureTelemetryHandler(handler);
#endif

builder.PrimaryHandler = handler;
}
Expand Down
23 changes: 0 additions & 23 deletions src/Shared/HttpHandlerFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,27 +50,4 @@ public static HttpMessageHandler CreatePrimaryHandler()
throw new PlatformNotSupportedException(message);
#endif
}

#if NET5_0
public static HttpMessageHandler EnsureTelemetryHandler(HttpMessageHandler handler)
{
// HttpClientHandler has an internal handler that sets request telemetry header.
// If the handler is SocketsHttpHandler then we know that the header will never be set
// so wrap with a handler that is responsible for setting the telemetry header.
if (HttpRequestHelpers.HasHttpHandlerType<SocketsHttpHandler>(handler))
{
// Double check telemetry handler hasn't already been added by something else
// like the client factory when it created the primary handler.
//
// Check with type name because this handler can come from shared source
// in multiple assemblies.
if (!HttpRequestHelpers.HasHttpHandlerType(handler, typeof(TelemetryHeaderHandler).FullName!))
{
return new TelemetryHeaderHandler(handler);
}
}

return handler;
}
#endif
}
Loading
Loading