Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Next Next commit
adjust logic to current main, log socketaddress to metrics
  • Loading branch information
antonfirsov committed Jul 16, 2023
commit d6ac30fd5571ca434db0ceb16d7612ae5b3a022c
22 changes: 11 additions & 11 deletions src/libraries/System.Net.Http/src/System/Net/Http/HttpTelemetry.cs
Original file line number Diff line number Diff line change
Expand Up @@ -91,16 +91,16 @@ private void RequestFailed(string exceptionMessage)
}

[NonEvent]
private void ConnectionEstablished(byte versionMajor, byte versionMinor, long connectionId, Uri uri, IPEndPoint? remoteEndPoint)
private void ConnectionEstablished(byte versionMajor, byte versionMinor, long connectionId, string scheme, string host, int port, IPEndPoint? remoteEndPoint)
{
string? remoteAddress = remoteEndPoint?.Address?.ToString();
ConnectionEstablished(versionMajor, versionMinor, connectionId, uri.Scheme, uri.Host, uri.Port, remoteAddress);
string? socketAddress = remoteEndPoint?.Address?.ToString();
ConnectionEstablished(versionMajor, versionMinor, connectionId, scheme, host, port, socketAddress);
}

[Event(4, Level = EventLevel.Informational)]
private void ConnectionEstablished(byte versionMajor, byte versionMinor, long connectionId, string scheme, string host, int port, string? remoteAddress)
private void ConnectionEstablished(byte versionMajor, byte versionMinor, long connectionId, string scheme, string host, int port, string? socketAddress)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also going with the socketAddress name here.

Copy link
Member

Choose a reason for hiding this comment

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

A more generic remoteEndPoint may be better if we do decide to allow users to customize the string that is logged here (e.g. they give us a custom stream via ConnectCallback that may not be backed by a socket at all).

{
WriteEvent(eventId: 4, versionMajor, versionMinor, connectionId, scheme, host, port, remoteAddress);
WriteEvent(eventId: 4, versionMajor, versionMinor, connectionId, scheme, host, port, socketAddress);
}

[Event(5, Level = EventLevel.Informational)]
Expand Down Expand Up @@ -176,10 +176,10 @@ public void Redirect(string redirectUri)
}

[NonEvent]
public void Http11ConnectionEstablished(long connectionId, Uri uri, IPEndPoint? remoteEndPoint)
public void Http11ConnectionEstablished(long connectionId, string scheme, string host, int port, IPEndPoint? remoteEndPoint)
{
Interlocked.Increment(ref _openedHttp11Connections);
ConnectionEstablished(versionMajor: 1, versionMinor: 1, connectionId, uri, remoteEndPoint);
ConnectionEstablished(versionMajor: 1, versionMinor: 1, connectionId, scheme, host, port, remoteEndPoint);
}

[NonEvent]
Expand All @@ -191,10 +191,10 @@ public void Http11ConnectionClosed(long connectionId)
}

[NonEvent]
public void Http20ConnectionEstablished(long connectionId, Uri uri, IPEndPoint? remoteEndPoint)
public void Http20ConnectionEstablished(long connectionId, string scheme, string host, int port, IPEndPoint? remoteEndPoint)
{
Interlocked.Increment(ref _openedHttp20Connections);
ConnectionEstablished(versionMajor: 2, versionMinor: 0, connectionId, uri, remoteEndPoint);
ConnectionEstablished(versionMajor: 2, versionMinor: 0, connectionId, scheme, host, port, remoteEndPoint);
}

[NonEvent]
Expand All @@ -206,10 +206,10 @@ public void Http20ConnectionClosed(long connectionId)
}

[NonEvent]
public void Http30ConnectionEstablished(long connectionId, Uri uri, IPEndPoint? remoteEndPoint)
public void Http30ConnectionEstablished(long connectionId, string scheme, string host, int port, IPEndPoint? remoteEndPoint)
{
Interlocked.Increment(ref _openedHttp30Connections);
ConnectionEstablished(versionMajor: 3, versionMinor: 0, connectionId, uri, remoteEndPoint);
ConnectionEstablished(versionMajor: 3, versionMinor: 0, connectionId, scheme, host, port, remoteEndPoint);
}

[NonEvent]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,8 @@ internal enum KeepAliveState
private long _keepAlivePingTimeoutTimestamp;
private volatile KeepAliveState _keepAliveState;

public Http2Connection(HttpConnectionPool pool, Stream stream)
: base(pool)
public Http2Connection(HttpConnectionPool pool, Stream stream, IPEndPoint? remoteEndPoint)
: base(pool, remoteEndPoint)
{
_pool = pool;
_stream = stream;
Expand Down Expand Up @@ -1656,7 +1656,7 @@ private async ValueTask<Http2Stream> SendHeadersAsync(HttpRequestMessage request
ArrayBuffer headerBuffer = default;
try
{
if (HttpTelemetry.Log.IsEnabled()) HttpTelemetry.Log.RequestHeadersStart();
if (HttpTelemetry.Log.IsEnabled()) HttpTelemetry.Log.RequestHeadersStart(Id);

// Serialize headers to a temporary buffer, and do as much work to prepare to send the headers as we can
// before taking the write lock.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ private bool ShuttingDown
}

public Http3Connection(HttpConnectionPool pool, HttpAuthority authority, QuicConnection connection, bool includeAltUsedHeader)
: base(pool)
: base(pool, connection.RemoteEndPoint)
{
_pool = pool;
_authority = authority;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,9 @@ internal sealed partial class HttpConnection : HttpConnectionBase
public HttpConnection(
HttpConnectionPool pool,
Stream stream,
TransportContext? transportContext)
: base(pool)
TransportContext? transportContext,
IPEndPoint? remoteEndPoint)
: base(pool, remoteEndPoint)
{
Debug.Assert(pool != null);
Debug.Assert(stream != null);
Expand Down Expand Up @@ -517,7 +518,7 @@ public async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, boo
CancellationTokenRegistration cancellationRegistration = RegisterCancellation(cancellationToken);
try
{
if (HttpTelemetry.Log.IsEnabled()) HttpTelemetry.Log.RequestHeadersStart();
if (HttpTelemetry.Log.IsEnabled()) HttpTelemetry.Log.RequestHeadersStart(Id);

WriteHeaders(request, normalizedMethod);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ internal abstract class HttpConnectionBase : IDisposable, IHttpTrace
/// <summary>Cached string for the last Server header received on this connection.</summary>
private string? _lastServerHeaderValue;

internal long Id { get; } = Interlocked.Increment(ref s_connectionCounter);
public long Id { get; } = Interlocked.Increment(ref s_connectionCounter);

public HttpConnectionBase(HttpConnectionPool pool)
public HttpConnectionBase(HttpConnectionPool pool, IPEndPoint? remoteEndPoint)
{
Debug.Assert(this is HttpConnection or Http2Connection or Http3Connection);
Debug.Assert(pool.Settings._metrics is not null);
Expand All @@ -59,7 +59,8 @@ public HttpConnectionBase(HttpConnectionPool pool)
protocol,
pool.IsSecure ? "https" : "http",
pool.OriginAuthority.HostValue,
port == defaultPort ? null : port);
port == defaultPort ? null : port,
remoteEndPoint?.Address?.ToString());

_connectionMetrics.ConnectionEstablished();

Expand All @@ -70,9 +71,13 @@ public HttpConnectionBase(HttpConnectionPool pool)
{
_httpTelemetryMarkedConnectionAsOpened = true;

if (this is HttpConnection) HttpTelemetry.Log.Http11ConnectionEstablished();
else if (this is Http2Connection) HttpTelemetry.Log.Http20ConnectionEstablished();
else HttpTelemetry.Log.Http30ConnectionEstablished();
string scheme = pool.IsSecure ? "https" : "http";
string host = pool.OriginAuthority.HostValue;
int port = pool.OriginAuthority.Port;

if (this is HttpConnection) HttpTelemetry.Log.Http11ConnectionEstablished(Id, scheme, host, port, remoteEndPoint);
else if (this is Http2Connection) HttpTelemetry.Log.Http20ConnectionEstablished(Id, scheme, host, port, remoteEndPoint);
else HttpTelemetry.Log.Http30ConnectionEstablished(Id, scheme, host, port, remoteEndPoint);
}
}

Expand All @@ -85,9 +90,9 @@ public void MarkConnectionAsClosed()
// Only decrement the connection count if we counted this connection
if (_httpTelemetryMarkedConnectionAsOpened)
{
if (this is HttpConnection) HttpTelemetry.Log.Http11ConnectionClosed();
else if (this is Http2Connection) HttpTelemetry.Log.Http20ConnectionClosed();
else HttpTelemetry.Log.Http30ConnectionClosed();
if (this is HttpConnection) HttpTelemetry.Log.Http11ConnectionClosed(Id);
else if (this is Http2Connection) HttpTelemetry.Log.Http20ConnectionClosed(Id);
else HttpTelemetry.Log.Http30ConnectionClosed(Id);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.IO;
using System.Net.Http.Headers;
using System.Net.Http.HPack;
using System.Net.Http.Metrics;
using System.Net.Http.QPack;
using System.Net.Quic;
using System.Net.Security;
Expand Down Expand Up @@ -955,7 +956,6 @@ private async ValueTask<Http3Connection> GetHttp3ConnectionAsync(HttpRequestMess
#endif
// if the authority was sent as an option through alt-svc then include alt-used header
http3Connection = new Http3Connection(this, authority, quicConnection, includeAltUsedHeader: _http3Authority == authority);
http3Connection = new Http3Connection(this, _originAuthority, authority, quicConnection, request.RequestUri, includeAltUsedHeader: _http3Authority == authority);
_http3Connection = http3Connection;

if (NetEventSource.Log.IsEnabled())
Expand Down Expand Up @@ -1544,11 +1544,8 @@ public ValueTask<HttpResponseMessage> SendAsync(HttpRequestMessage request, bool
case HttpConnectionKind.Https:
case HttpConnectionKind.ProxyConnect:
stream = await ConnectToTcpHostAsync(_originAuthority.IdnHost, _originAuthority.Port, request, async, cancellationToken).ConfigureAwait(false);
if (HttpTelemetry.Log.IsEnabled())
{
// remoteEndPoint is returned optionally for diagnostic purposes.
remoteEndPoint = GetRemoteEndPoint(stream);
}
// remoteEndPoint is returned for diagnostic purposes.
remoteEndPoint = GetRemoteEndPoint(stream);
if (_kind == HttpConnectionKind.ProxyConnect && _sslOptionsProxy != null)
{
stream = await ConnectHelper.EstablishSslConnectionAsync(_sslOptionsProxy, request, async, stream, cancellationToken).ConfigureAwait(false);
Expand All @@ -1557,11 +1554,8 @@ public ValueTask<HttpResponseMessage> SendAsync(HttpRequestMessage request, bool

case HttpConnectionKind.Proxy:
stream = await ConnectToTcpHostAsync(_proxyUri!.IdnHost, _proxyUri.Port, request, async, cancellationToken).ConfigureAwait(false);
if (HttpTelemetry.Log.IsEnabled())
{
// remoteEndPoint is returned optionally for diagnostic purposes.
remoteEndPoint = GetRemoteEndPoint(stream);
}
// remoteEndPoint is returned for diagnostic purposes.
remoteEndPoint = GetRemoteEndPoint(stream);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm no longer checking for any telemetry || metrics being enabled here, since GetRemoteEndpoint is relatively cheap to run per-connection and likely almost as expensive as the check would be.

if (_sslOptionsProxy != null)
{
stream = await ConnectHelper.EstablishSslConnectionAsync(_sslOptionsProxy, request, async, stream, cancellationToken).ConfigureAwait(false);
Expand Down Expand Up @@ -1735,16 +1729,14 @@ private async ValueTask<Stream> ApplyPlaintextFilterAsync(bool async, Stream str
private async ValueTask<HttpConnection> ConstructHttp11ConnectionAsync(bool async, Stream stream, TransportContext? transportContext, HttpRequestMessage request, IPEndPoint? remoteEndPoint, CancellationToken cancellationToken)
{
Stream newStream = await ApplyPlaintextFilterAsync(async, stream, HttpVersion.Version11, request, cancellationToken).ConfigureAwait(false);
Debug.Assert(request.RequestUri != null);
return new HttpConnection(this, newStream, transportContext, request.RequestUri, remoteEndPoint);
return new HttpConnection(this, newStream, transportContext, remoteEndPoint);
}

private async ValueTask<Http2Connection> ConstructHttp2ConnectionAsync(Stream stream, HttpRequestMessage request, IPEndPoint? remoteEndPoint, CancellationToken cancellationToken)
{
stream = await ApplyPlaintextFilterAsync(async: true, stream, HttpVersion.Version20, request, cancellationToken).ConfigureAwait(false);

Debug.Assert(request.RequestUri != null);
Http2Connection http2Connection = new Http2Connection(this, stream, request.RequestUri, remoteEndPoint);
Http2Connection http2Connection = new Http2Connection(this, stream, remoteEndPoint);
try
{
await http2Connection.SetupAsync(cancellationToken).ConfigureAwait(false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,10 @@ internal sealed class ConnectionMetrics
private readonly object _schemeTag;
private readonly object _hostTag;
private readonly object? _portTag;
private readonly string? _socketAddress;
private bool _currentlyIdle;

public ConnectionMetrics(SocketsHttpHandlerMetrics metrics, string protocol, string scheme, string host, int? port)
public ConnectionMetrics(SocketsHttpHandlerMetrics metrics, string protocol, string scheme, string host, int? port, string? socketAddress)
{
_metrics = metrics;
_currentConnectionsEnabled = _metrics.CurrentConnections.Enabled;
Expand All @@ -25,6 +26,7 @@ public ConnectionMetrics(SocketsHttpHandlerMetrics metrics, string protocol, str
_schemeTag = scheme;
_hostTag = host;
_portTag = port;
_socketAddress = socketAddress;
}

// TagList is a huge struct, so we avoid storing it in a field to reduce the amount we allocate on the heap.
Expand All @@ -41,6 +43,11 @@ private TagList GetTags()
tags.Add("port", _portTag);
}

if (_socketAddress is not null)
{
tags.Add("socket.address", _socketAddress);
}
Copy link
Contributor Author

@antonfirsov antonfirsov Jul 16, 2023

Choose a reason for hiding this comment

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

Edit: Reverted this to simplify Metrics naming discussions. If this is interesting enough to be added after platform complete, we can do it in a follow-up PR.

Given it's available, it looks like it makes make sense to log this also on connection metrics, unless we think it's sensitive info. With the name I'm following OTel recommendation proactively using dot notation with the assumption that we will also change the instrument names by midnight of the 17th.

@davidfowl @noahfalk any objections?


return tags;
}

Expand Down
21 changes: 15 additions & 6 deletions src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,20 @@ protected static void VerifyOptionalTag<T>(KeyValuePair<string, object?>[] tags,
}
}

private static void VerifySchemeHostPortTags(KeyValuePair<string, object?>[] tags, Uri uri)
private static void VerifySchemeHostPortSocketAddressTags(KeyValuePair<string, object?>[] tags, Uri uri, bool verifySocketAddress = false)
{
VerifyOptionalTag(tags, "scheme", uri.Scheme);
VerifyOptionalTag(tags, "host", uri.Host);
VerifyOptionalTag(tags, "port", uri.Port);

if (verifySocketAddress)
{
string socketAddress = (string)tags.Single(t => t.Key == "socket.address").Value;
IPAddress ip = IPAddress.Parse(socketAddress);
Assert.True(ip.Equals(IPAddress.Loopback.MapToIPv6()) ||
ip.Equals(IPAddress.Loopback) ||
ip.Equals(IPAddress.IPv6Loopback));
}
}

protected static void VerifyRequestDuration(Measurement<double> measurement, Uri uri, string? protocol, int? statusCode, string method = "GET") =>
Expand All @@ -60,7 +69,7 @@ protected static void VerifyRequestDuration(string instrumentName, double measur
{
Assert.Equal(InstrumentNames.RequestDuration, instrumentName);
Assert.InRange(measurement, double.Epsilon, 60);
VerifySchemeHostPortTags(tags, uri);
VerifySchemeHostPortSocketAddressTags(tags, uri);
VerifyOptionalTag(tags, "method", method);
VerifyOptionalTag(tags, "protocol", protocol);
VerifyOptionalTag(tags, "status-code", statusCode);
Expand All @@ -73,7 +82,7 @@ protected static void VerifyCurrentRequest(string instrumentName, long measureme
{
Assert.Equal(InstrumentNames.CurrentRequests, instrumentName);
Assert.Equal(expectedValue, measurement);
VerifySchemeHostPortTags(tags, uri);
VerifySchemeHostPortSocketAddressTags(tags, uri);
}

protected static void VerifyFailedRequests(Measurement<long> measurement, long expectedValue, Uri uri, string? protocol, int? statusCode, string method = "GET")
Expand All @@ -82,7 +91,7 @@ protected static void VerifyFailedRequests(Measurement<long> measurement, long e

KeyValuePair<string, object?>[] tags = measurement.Tags.ToArray();

VerifySchemeHostPortTags(tags, uri);
VerifySchemeHostPortSocketAddressTags(tags, uri);

Assert.Equal(method, tags.Single(t => t.Key == "method").Value);
VerifyOptionalTag(tags, "protocol", protocol);
Expand All @@ -93,15 +102,15 @@ protected static void VerifyConnectionCounter(string expectedName, string actual
{
Assert.Equal(expectedName, actualName);
Assert.Equal(expectedValue, Assert.IsType<long>(measurement));
VerifySchemeHostPortTags(tags, uri);
VerifySchemeHostPortSocketAddressTags(tags, uri, verifySocketAddress: true);
VerifyOptionalTag(tags, "protocol", protocol);
}

protected static void VerifyConnectionDuration(string instrumentName, double measurement, KeyValuePair<string, object?>[] tags, Uri uri, string protocol)
{
Assert.InRange(measurement, double.Epsilon, 60);
Assert.Equal(InstrumentNames.ConnectionDuration, instrumentName);
VerifySchemeHostPortTags(tags, uri);
VerifySchemeHostPortSocketAddressTags(tags, uri, verifySocketAddress: true);
VerifyOptionalTag(tags, "protocol", protocol);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,7 @@ protected static void ValidateConnectionEstablishedClosed(ConcurrentQueue<(Event
foreach (EventWrittenEventArgs connectionEstablished in connectionsEstablished)
{
Assert.Equal(7, connectionEstablished.Payload.Count);
Assert.Equal(new[] { "versionMajor", "versionMinor", "connectionId", "scheme", "host", "port", "remoteAddress" }, connectionEstablished.PayloadNames);
Assert.Equal(new[] { "versionMajor", "versionMinor", "connectionId", "scheme", "host", "port", "socketAddress" }, connectionEstablished.PayloadNames);
Assert.Equal(version.Major, (byte)connectionEstablished.Payload[0]);
Assert.Equal(version.Minor, (byte)connectionEstablished.Payload[1]);
long connectionId = (long)connectionEstablished.Payload[2];
Expand Down