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.

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

Wire up DiagnosticsHandler #456

Merged
merged 22 commits into from
Nov 12, 2020
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ public sealed class ProxyHttpClientOptions

public int? MaxConnectionsPerServer { get; set; }

public bool PropagateActivityContext { get; set; }
jmezach marked this conversation as resolved.
Show resolved Hide resolved

// TODO: Add this property once we have migrated to SDK version that supports it.
//public bool? EnableMultipleHttp2Connections { get; set; }

Expand All @@ -28,7 +30,8 @@ internal ProxyHttpClientOptions DeepClone()
DangerousAcceptAnyServerCertificate = DangerousAcceptAnyServerCertificate,
// TODO: Clone certificate?
ClientCertificate = ClientCertificate,
MaxConnectionsPerServer = MaxConnectionsPerServer
MaxConnectionsPerServer = MaxConnectionsPerServer,
PropagateActivityContext = PropagateActivityContext,
};
}

Expand All @@ -47,7 +50,8 @@ internal static bool Equals(ProxyHttpClientOptions options1, ProxyHttpClientOpti
return options1.SslProtocols == options2.SslProtocols
&& Equals(options1.ClientCertificate, options2.ClientCertificate)
&& options1.DangerousAcceptAnyServerCertificate == options2.DangerousAcceptAnyServerCertificate
&& options1.MaxConnectionsPerServer == options2.MaxConnectionsPerServer;
&& options1.MaxConnectionsPerServer == options2.MaxConnectionsPerServer
&& options1.PropagateActivityContext == options2.PropagateActivityContext;
}

private static bool Equals(X509Certificate2 certificate1, X509Certificate2 certificate2)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,8 @@ private ProxyHttpClientOptions Convert(ProxyHttpClientData data)
SslProtocols = sslProtocols,
DangerousAcceptAnyServerCertificate = data.DangerousAcceptAnyServerCertificate,
ClientCertificate = clientCertificate,
MaxConnectionsPerServer = data.MaxConnectionsPerServer
MaxConnectionsPerServer = data.MaxConnectionsPerServer,
PropagateActivityContext = data.PropagateActivityContext
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ public sealed class ProxyHttpClientData

public int? MaxConnectionsPerServer { get; set; }

public bool PropagateActivityContext { get; set; }

// TODO: Add this property once we have migrated to SDK version that supports it.
//public bool? EnableMultipleHttp2Connections { get; set; }
}
Expand Down
3 changes: 2 additions & 1 deletion src/ReverseProxy/Service/Management/ProxyConfigManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -503,7 +503,8 @@ private ClusterProxyHttpClientOptions ConvertProxyHttpClientOptions(ProxyHttpCli
httpClientOptions.SslProtocols,
httpClientOptions.DangerousAcceptAnyServerCertificate,
httpClientOptions.ClientCertificate,
httpClientOptions.MaxConnectionsPerServer);
httpClientOptions.MaxConnectionsPerServer,
httpClientOptions.PropagateActivityContext);
}

public void Dispose()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Net.Http;
using System.Security.Cryptography.X509Certificates;
using Microsoft.Extensions.Logging;
using Microsoft.ReverseProxy.Telemetry;

namespace Microsoft.ReverseProxy.Service.Proxy.Infrastructure
{
Expand Down Expand Up @@ -65,7 +66,7 @@ public HttpMessageInvoker CreateClient(ProxyHttpClientContext context)
}

Log.ProxyClientCreated(_logger, context.ClusterId);
return new HttpMessageInvoker(handler, disposeHandler: true);
return new HttpMessageInvoker(new DiagnosticsHandler(handler, newClientOptions.PropagateActivityContext), disposeHandler: true);
jmezach marked this conversation as resolved.
Show resolved Hide resolved
}

private bool CanReuseOldClient(ProxyHttpClientContext context)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,14 @@ public ClusterProxyHttpClientOptions(
SslProtocols? sslProtocols,
bool acceptAnyServerCertificate,
X509Certificate2 clientCertificate,
int? maxConnectionsPerServer)
int? maxConnectionsPerServer,
bool propagateActivityContext)
{
SslProtocols = sslProtocols;
DangerousAcceptAnyServerCertificate = acceptAnyServerCertificate;
ClientCertificate = clientCertificate;
MaxConnectionsPerServer = maxConnectionsPerServer;
PropagateActivityContext = propagateActivityContext;
}

public SslProtocols? SslProtocols { get; }
Expand All @@ -30,6 +32,8 @@ public ClusterProxyHttpClientOptions(

public int? MaxConnectionsPerServer { get; }

public bool PropagateActivityContext { get; }

// TODO: Add this property once we have migrated to SDK version that supports it.
//public bool? EnableMultipleHttp2Connections { get; }

Expand All @@ -43,7 +47,8 @@ public bool Equals(ClusterProxyHttpClientOptions other)
return SslProtocols == other.SslProtocols &&
DangerousAcceptAnyServerCertificate == other.DangerousAcceptAnyServerCertificate &&
EqualityComparer<X509Certificate2>.Default.Equals(ClientCertificate, other.ClientCertificate) &&
MaxConnectionsPerServer == other.MaxConnectionsPerServer;
MaxConnectionsPerServer == other.MaxConnectionsPerServer &&
PropagateActivityContext == other.PropagateActivityContext;
}

public override int GetHashCode()
Expand Down
119 changes: 119 additions & 0 deletions src/ReverseProxy/Telemetry/DiagnosticsHandler.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Net.Http;
using System.Net.Http.Headers;
using System.Threading;
using System.Threading.Tasks;

namespace Microsoft.ReverseProxy.Telemetry
{
/// <summary>
/// DiagnosticHandler notifies DiagnosticSource subscribers about outgoing Http requests
/// </summary>
internal sealed class DiagnosticsHandler : DelegatingHandler
{
private readonly bool _injectHeaders;

/// <summary>
/// DiagnosticHandler constructor
/// </summary>
/// <param name="innerHandler">Inner handler: Windows or Unix implementation of HttpMessageHandler.
/// Note that DiagnosticHandler is the latest in the pipeline </param>
/// <param name="injectHeaders">Indicates whether headers will be injected to enable distributed tracing.</param>
public DiagnosticsHandler(HttpMessageHandler innerHandler, bool injectHeaders) : base(innerHandler)
{
_injectHeaders = injectHeaders;
}

protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request,
jmezach marked this conversation as resolved.
Show resolved Hide resolved
CancellationToken cancellationToken)
{
jmezach marked this conversation as resolved.
Show resolved Hide resolved
// HttpClientHandler is responsible to call static DiagnosticsHandler.IsEnabled() before forwarding request here.
// It will check if propagation is on (because parent Activity exists or there is a listener) or off (forcibly disabled)
// This code won't be called unless consumer unsubscribes from DiagnosticListener right after the check.
// So some requests happening right after subscription starts might not be instrumented. Similarly,
// when consumer unsubscribes, extra requests might be instrumented

if (request == null)
{
throw new ArgumentNullException(nameof(request)); //, SR.net_http_handler_norequest); TODO: Is this really necessary?
}

// If we are on at all, we propagate current activity information
jmezach marked this conversation as resolved.
Show resolved Hide resolved
var currentActivity = Activity.Current;
if (currentActivity != null)
{
InjectHeaders(currentActivity, request);
}

try
{
var responseTask = base.SendAsync(request, cancellationToken);

return await responseTask.ConfigureAwait(false);
}
catch (OperationCanceledException)
{
// we'll report task status in HttpRequestOut.Stop
throw;
}
}
jmezach marked this conversation as resolved.
Show resolved Hide resolved

#region private

private void InjectHeaders(Activity currentActivity, HttpRequestMessage request)
{
if (!_injectHeaders)
{
// If we're not enabled we don't have anything to do here
return;
}

if (currentActivity.IdFormat == ActivityIdFormat.W3C)
{
request.Headers.Remove(DiagnosticsHandlerLoggingStrings.TraceParentHeaderName);

if (!request.Headers.Contains(DiagnosticsHandlerLoggingStrings.TraceParentHeaderName))
jmezach marked this conversation as resolved.
Show resolved Hide resolved
{
request.Headers.TryAddWithoutValidation(DiagnosticsHandlerLoggingStrings.TraceParentHeaderName, currentActivity.Id);
if (currentActivity.TraceStateString != null)
{
request.Headers.TryAddWithoutValidation(DiagnosticsHandlerLoggingStrings.TraceStateHeaderName, currentActivity.TraceStateString);
}
}
}
else
{
request.Headers.Remove(DiagnosticsHandlerLoggingStrings.RequestIdHeaderName);

if (!request.Headers.Contains(DiagnosticsHandlerLoggingStrings.RequestIdHeaderName))
jmezach marked this conversation as resolved.
Show resolved Hide resolved
{
request.Headers.TryAddWithoutValidation(DiagnosticsHandlerLoggingStrings.RequestIdHeaderName, currentActivity.Id);
}
}

// we expect baggage to be empty or contain a few items
using (var e = currentActivity.Baggage.GetEnumerator())
{
if (e.MoveNext())
{
var baggage = new List<string>();
do
{
var item = e.Current;
baggage.Add(new NameValueHeaderValue(Uri.EscapeDataString(item.Key), Uri.EscapeDataString(item.Value)).ToString());
}
while (e.MoveNext());
request.Headers.TryAddWithoutValidation(DiagnosticsHandlerLoggingStrings.CorrelationContextHeaderName, baggage);
}
}
}

#endregion
}
}
19 changes: 19 additions & 0 deletions src/ReverseProxy/Telemetry/DiagnosticsHandlerLoggingStrings.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
namespace Microsoft.ReverseProxy.Telemetry
{
/// <summary>
/// Defines names of DiagnosticListener and Write events for WinHttpHandler, CurlHandler, and HttpHandlerToFilter.
/// </summary>
internal static class DiagnosticsHandlerLoggingStrings
{
public const string DiagnosticListenerName = "HttpHandlerDiagnosticListener";
public const string ExceptionEventName = "System.Net.Http.Exception";
public const string ActivityName = "System.Net.Http.HttpRequestOut";
public const string ActivityStartName = "System.Net.Http.HttpRequestOut.Start";

public const string RequestIdHeaderName = "Request-Id";
public const string CorrelationContextHeaderName = "Correlation-Context";

public const string TraceParentHeaderName = "traceparent";
public const string TraceStateHeaderName = "tracestate";
}
}
jmezach marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,8 @@ public class ConfigurationConfigProviderTests
SslProtocols = new List<SslProtocols> { SslProtocols.Tls11, SslProtocols.Tls12 },
MaxConnectionsPerServer = 10,
DangerousAcceptAnyServerCertificate = true,
ClientCertificate = new CertificateConfigData { Path = "mycert.pfx", Password = "myPassword1234" }
ClientCertificate = new CertificateConfigData { Path = "mycert.pfx", Password = "myPassword1234" },
PropagateActivityContext = true,
},
Metadata = new Dictionary<string, string> { { "cluster1-K1", "cluster1-V1" }, { "cluster1-K2", "cluster1-V2" } }
}
Expand Down Expand Up @@ -197,7 +198,8 @@ public class ConfigurationConfigProviderTests
""Location"": null,
""AllowInvalid"": null
},
""MaxConnectionsPerServer"": 10
""MaxConnectionsPerServer"": 10,
""PropagateActivityContext"": true,
},
""Destinations"": {
""destinationA"": {
Expand Down
Loading