Skip to content

Commit 34f175b

Browse files
JamesNKJamesNK
committed
Clean up
1 parent cc3a26f commit 34f175b

24 files changed

+107
-232
lines changed

‎src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,6 @@ private static int GetMaximumEnhanceYourCalmCount()
8787

8888
private readonly HttpConnectionContext _context;
8989
private readonly ConnectionMetricsContext _metricsContext;
90-
private readonly IProtocolErrorCodeFeature _errorCodeFeature;
9190
private readonly IConnectionMetricsTagsFeature? _metricsTagsFeature;
9291
private readonly Http2FrameWriter _frameWriter;
9392
private readonly Pipe _input;
@@ -142,7 +141,6 @@ public Http2Connection(HttpConnectionContext context)
142141
_context = context;
143142
_streamLifetimeHandler = this;
144143
_metricsContext = context.MetricsContext;
145-
_errorCodeFeature = context.ConnectionFeatures.GetRequiredFeature<IProtocolErrorCodeFeature>();
146144
_metricsTagsFeature = context.ConnectionFeatures.Get<IConnectionMetricsTagsFeature>();
147145

148146
// Capture the ExecutionContext before dispatching HTTP/2 middleware. Will be restored by streams when processing request
@@ -221,9 +219,7 @@ public void OnInputOrOutputCompleted()
221219
private void SetConnectionErrorCode(ConnectionEndReason reason, Http2ErrorCode errorCode)
222220
{
223221
Debug.Assert(_isClosed == 1, "Should only be set when connection is closed.");
224-
Debug.Assert(_errorCodeFeature.Error == -1, "Error code feature should only be set once.");
225222

226-
_errorCodeFeature.Error = (long)errorCode;
227223
KestrelMetrics.AddConnectionEndReason(_metricsContext, reason);
228224
}
229225

‎src/Servers/Kestrel/Core/src/Internal/HttpConnection.cs

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,6 @@ public async Task ProcessRequestsAsync<TContext>(IHttpApplication<TContext> http
6868
// _http2Connection must be initialized before yielding control to the transport thread,
6969
// to prevent a race condition where _http2Connection.Abort() is called just as
7070
// _http2Connection is about to be initialized.
71-
_context.ConnectionFeatures.Set<IProtocolErrorCodeFeature>(new ProtocolErrorCodeFeature());
7271
requestProcessor = new Http2Connection((HttpConnectionContext)_context);
7372
_protocolSelectionState = ProtocolSelectionState.Selected;
7473
AddMetricsHttpProtocolTag(KestrelMetrics.Http2);
@@ -119,18 +118,14 @@ public async Task ProcessRequestsAsync<TContext>(IHttpApplication<TContext> http
119118
}
120119
finally
121120
{
121+
// Before exiting HTTP layer, set the end reason on the context as a connection metrics tag.
122122
if (_context.MetricsContext.ConnectionEndReason is { } connectionEndReason)
123123
{
124124
KestrelMetrics.AddConnectionEndReason(connectionMetricsTagsFeature, connectionEndReason);
125125
}
126126
}
127127
}
128128

129-
private sealed class ProtocolErrorCodeFeature : IProtocolErrorCodeFeature
130-
{
131-
public long Error { get; set; } = -1;
132-
}
133-
134129
private void AddMetricsHttpProtocolTag(string httpVersion)
135130
{
136131
if (_context.ConnectionContext.Features.Get<IConnectionMetricsTagsFeature>() is { } metricsTags)

‎src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelMetrics.cs

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -102,18 +102,12 @@ public void ConnectionStop(in ConnectionMetricsContext metricsContext, Exception
102102
{
103103
if (metricsContext.CurrentConnectionsCounterEnabled || metricsContext.ConnectionDurationEnabled)
104104
{
105-
// Add protocol error code if feature is available and it's not the unset value (-1).
106-
long? errorCode = null;
107-
if (metricsContext.ConnectionContext.Features.Get<IProtocolErrorCodeFeature>() is IProtocolErrorCodeFeature errorCodeFeature && errorCodeFeature.Error != -1)
108-
{
109-
errorCode = errorCodeFeature.Error;
110-
}
111-
ConnectionStopCore(metricsContext, exception, errorCode, customTags, startTimestamp, currentTimestamp);
105+
ConnectionStopCore(metricsContext, exception, customTags, startTimestamp, currentTimestamp);
112106
}
113107
}
114108

115109
[MethodImpl(MethodImplOptions.NoInlining)]
116-
private void ConnectionStopCore(in ConnectionMetricsContext metricsContext, Exception? exception, long? protocolErrorCode, List<KeyValuePair<string, object?>>? customTags, long startTimestamp, long currentTimestamp)
110+
private void ConnectionStopCore(in ConnectionMetricsContext metricsContext, Exception? exception, List<KeyValuePair<string, object?>>? customTags, long startTimestamp, long currentTimestamp)
117111
{
118112
var tags = new TagList();
119113
InitializeConnectionTags(ref tags, metricsContext);
@@ -126,11 +120,6 @@ private void ConnectionStopCore(in ConnectionMetricsContext metricsContext, Exce
126120

127121
if (metricsContext.ConnectionDurationEnabled)
128122
{
129-
if (protocolErrorCode != null)
130-
{
131-
tags.Add("http.connection.protocol_code", protocolErrorCode);
132-
}
133-
134123
// Check if there is an end reason on the context. For example, the connection could have been aborted by shutdown.
135124
if (metricsContext.ConnectionEndReason is { } reason && TryGetErrorType(reason, out var errorValue))
136125
{

‎src/Servers/Kestrel/shared/test/Http3/Http3InMemory.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ public void OnTimeout(TimeoutReason reason)
9393

9494
internal TestMultiplexedConnectionContext MultiplexedConnectionContext { get; set; }
9595

96-
internal IDictionary<string, object> ConnectionTags => MultiplexedConnectionContext.Tags.ToDictionary(t => t.Key, t => t.Value);
96+
internal Dictionary<string, object> ConnectionTags => MultiplexedConnectionContext.Tags.ToDictionary(t => t.Key, t => t.Value);
9797

9898
internal long GetStreamId(long mask)
9999
{

‎src/Servers/Kestrel/shared/test/MetricsAssert.cs

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using System;
5+
using Microsoft.AspNetCore.Server.Kestrel.Core;
6+
using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure;
7+
8+
namespace Microsoft.AspNetCore.InternalTesting;
9+
10+
internal static class MetricsAssert
11+
{
12+
public static void Equal(ConnectionEndReason expectedReason, string errorType)
13+
{
14+
Assert.Equal(KestrelMetrics.GetErrorType(expectedReason), errorType);
15+
}
16+
17+
public static void Equal(ConnectionEndReason expectedReason, IReadOnlyDictionary<string, object> tags)
18+
{
19+
Equal(expectedReason, (string) tags[KestrelMetrics.ErrorType]);
20+
}
21+
22+
public static void NoError(IReadOnlyDictionary<string, object> tags)
23+
{
24+
if (tags.TryGetValue(KestrelMetrics.ErrorType, out var error))
25+
{
26+
Assert.Fail($"Tag collection contains {KestrelMetrics.ErrorType} with value {error}.");
27+
}
28+
}
29+
}

‎src/Servers/Kestrel/test/FunctionalTests/RequestTests.cs

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -624,10 +624,7 @@ public async Task RequestAbortedTokenFiredOnClientFIN()
624624
await host.StopAsync();
625625
}
626626

627-
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m =>
628-
{
629-
Assert.DoesNotContain(KestrelMetrics.ErrorType, m.Tags.Keys);
630-
});
627+
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m => MetricsAssert.NoError(m.Tags));
631628
}
632629

633630
[Fact]
@@ -727,10 +724,7 @@ public async Task AbortingTheConnection(bool fin)
727724
await host.StopAsync();
728725
}
729726

730-
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m =>
731-
{
732-
Assert.Equal(KestrelMetrics.GetErrorType(ConnectionEndReason.AbortedByApp), (string)m.Tags[KestrelMetrics.ErrorType]);
733-
});
727+
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m => MetricsAssert.Equal(ConnectionEndReason.AbortedByApp, m.Tags));
734728
}
735729

736730
[Theory]

‎src/Servers/Kestrel/test/FunctionalTests/ResponseTests.cs

Lines changed: 5 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -253,10 +253,7 @@ await connection.Send(
253253
await requestAbortedWh.Task.DefaultTimeout();
254254
}
255255

256-
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m =>
257-
{
258-
Assert.DoesNotContain(KestrelMetrics.ErrorType, m.Tags.Keys);
259-
});
256+
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m => MetricsAssert.NoError(m.Tags));
260257
}
261258

262259
[Theory]
@@ -463,7 +460,7 @@ await connection.Send(
463460
await connectionDuration.WaitForMeasurementsAsync(minCount: 1).DefaultTimeout();
464461

465462
var measurement = connectionDuration.GetMeasurementSnapshot().First();
466-
Assert.DoesNotContain(KestrelMetrics.ErrorType, measurement.Tags.Keys);
463+
MetricsAssert.NoError(measurement.Tags);
467464
}
468465

469466
[Theory]
@@ -599,10 +596,7 @@ await connection.Send(
599596
}
600597
}
601598

602-
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m =>
603-
{
604-
Assert.Equal(KestrelMetrics.GetErrorType(ConnectionEndReason.MinResponseDataRate), (string)m.Tags[KestrelMetrics.ErrorType]);
605-
});
599+
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m => MetricsAssert.Equal(ConnectionEndReason.MinResponseDataRate, m.Tags));
606600
}
607601

608602
[Theory]
@@ -850,10 +844,7 @@ await connection.Send(
850844
}
851845
}
852846

853-
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m =>
854-
{
855-
Assert.Equal(KestrelMetrics.GetErrorType(ConnectionEndReason.MinResponseDataRate), (string)m.Tags[KestrelMetrics.ErrorType]);
856-
});
847+
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m => MetricsAssert.Equal(ConnectionEndReason.MinResponseDataRate, m.Tags));
857848
}
858849

859850
[ConditionalFact]
@@ -1094,10 +1085,7 @@ await connection.Receive(
10941085
Assert.Equal(1, TestSink.Writes.Count(w => w.EventId.Name == "ConnectionStop"));
10951086
Assert.False(requestAborted);
10961087

1097-
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m =>
1098-
{
1099-
Assert.DoesNotContain(KestrelMetrics.ErrorType, m.Tags.Keys);
1100-
});
1088+
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m => MetricsAssert.NoError(m.Tags));
11011089
}
11021090

11031091
private async Task AssertStreamAborted(Stream stream, int totalBytes)

‎src/Servers/Kestrel/test/InMemory.FunctionalTests/BadHttpRequestTests.cs

Lines changed: 9 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -183,10 +183,7 @@ public async Task CanOptOutOfBadRequestIfHostHeaderDoesNotMatchRequestTarget(str
183183

184184
Assert.Equal("www.foo.com:80", receivedHost);
185185

186-
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m =>
187-
{
188-
Assert.DoesNotContain(KestrelMetrics.ErrorType, m.Tags);
189-
});
186+
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m => MetricsAssert.NoError(m.Tags));
190187
}
191188

192189
[Fact]
@@ -249,10 +246,8 @@ await client.SendAll(
249246
}
250247
}
251248

252-
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m =>
253-
{
254-
Assert.Equal(KestrelMetrics.GetErrorType(ConnectionEndReason.InvalidRequestLine), m.Tags[KestrelMetrics.ErrorType]);
255-
});
249+
Assert.Collection(connectionDuration.GetMeasurementSnapshot(),
250+
m => MetricsAssert.Equal(ConnectionEndReason.InvalidRequestLine, m.Tags));
256251
}
257252

258253
[Fact]
@@ -274,10 +269,8 @@ public async Task BadRequestForHttp2()
274269
}
275270
}
276271

277-
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m =>
278-
{
279-
Assert.Equal(KestrelMetrics.GetErrorType(ConnectionEndReason.InvalidHttpVersion), m.Tags[KestrelMetrics.ErrorType]);
280-
});
272+
Assert.Collection(connectionDuration.GetMeasurementSnapshot(),
273+
m => MetricsAssert.Equal(ConnectionEndReason.InvalidHttpVersion, m.Tags));
281274
}
282275

283276
[Fact]
@@ -364,10 +357,7 @@ private async Task TestBadRequest(string request, string expectedResponseStatusC
364357
Assert.Equal("Microsoft.AspNetCore.Server.Kestrel.BadRequest", eventProviderName);
365358
Assert.Contains(expectedExceptionMessage, exceptionString);
366359

367-
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m =>
368-
{
369-
Assert.Equal(KestrelMetrics.GetErrorType(reason), m.Tags[KestrelMetrics.ErrorType]);
370-
});
360+
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m => MetricsAssert.Equal(reason, m.Tags));
371361
}
372362

373363
[Theory]
@@ -440,10 +430,7 @@ await connection.Receive(
440430
// Verify DiagnosticSource event for bad request
441431
Assert.False(badRequestEventListener.EventFired);
442432

443-
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m =>
444-
{
445-
Assert.DoesNotContain(KestrelMetrics.ErrorType, m.Tags);
446-
});
433+
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m => MetricsAssert.NoError(m.Tags));
447434
}
448435

449436
[Fact]
@@ -512,10 +499,7 @@ await connection.Receive(
512499
// Verify DiagnosticSource event for bad request
513500
Assert.False(badRequestEventListener.EventFired);
514501

515-
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m =>
516-
{
517-
Assert.DoesNotContain(KestrelMetrics.ErrorType, m.Tags);
518-
});
502+
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m => MetricsAssert.NoError(m.Tags));
519503
}
520504

521505
[Theory]
@@ -573,10 +557,7 @@ await connection.Receive(
573557
// Verify DiagnosticSource event for bad request
574558
Assert.False(badRequestEventListener.EventFired);
575559

576-
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m =>
577-
{
578-
Assert.DoesNotContain(KestrelMetrics.ErrorType, m.Tags);
579-
});
560+
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m => MetricsAssert.NoError(m.Tags));
580561
}
581562

582563
private async Task ReceiveBadRequestResponse(InMemoryConnection connection, string expectedResponseStatusCode, string expectedDateHeaderValue, string expectedAllowHeader = null)

‎src/Servers/Kestrel/test/InMemory.FunctionalTests/ChunkedRequestTests.cs

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -899,10 +899,7 @@ await connection.SendAll(
899899
}
900900
}
901901

902-
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m =>
903-
{
904-
Assert.Equal(KestrelMetrics.GetErrorType(ConnectionEndReason.UnexpectedEndOfRequestContent), m.Tags[KestrelMetrics.ErrorType]);
905-
});
902+
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m => MetricsAssert.Equal(ConnectionEndReason.UnexpectedEndOfRequestContent, m.Tags));
906903
}
907904

908905
[Fact]

‎src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5760,7 +5760,6 @@ public async Task StartConnection_SendHttp1xRequest_ReturnHttp11Status400()
57605760
Assert.NotNull(Http2Connection.InvalidHttp1xErrorResponseBytes);
57615761
Assert.Equal(Http2Connection.InvalidHttp1xErrorResponseBytes, data);
57625762
AssertConnectionEndReason(ConnectionEndReason.InvalidHttpVersion);
5763-
Assert.Equal(Http2ErrorCode.PROTOCOL_ERROR, (Http2ErrorCode)_errorCodeFeature.Error);
57645763
}
57655764

57665765
[Fact]

‎src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2TestBase.cs

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -157,12 +157,11 @@ protected static IEnumerable<KeyValuePair<string, string>> ReadRateRequestHeader
157157
internal DuplexPipe.DuplexPipePair _pair;
158158
internal IConnectionMetricsTagsFeature _metricsTagsFeature;
159159
internal IConnectionMetricsContextFeature _metricsContextFeature;
160-
internal IProtocolErrorCodeFeature _errorCodeFeature;
161160
internal Http2Connection _connection;
162161
protected Task _connectionTask;
163162
protected long _bytesReceived;
164163

165-
internal IDictionary<string, object> ConnectionTags => _metricsTagsFeature.Tags.ToDictionary(t => t.Key, t => t.Value);
164+
internal Dictionary<string, object> ConnectionTags => _metricsTagsFeature.Tags.ToDictionary(t => t.Key, t => t.Value);
166165
internal ConnectionMetricsContext MetricsContext => _metricsContextFeature.MetricsContext;
167166

168167
public Http2TestBase()
@@ -428,7 +427,7 @@ public override void Dispose()
428427

429428
internal void AssertConnectionNoError()
430429
{
431-
Assert.DoesNotContain(KestrelMetrics.ErrorType, ConnectionTags.Keys);
430+
MetricsAssert.NoError(ConnectionTags);
432431
}
433432

434433
internal void AssertConnectionEndReason(ConnectionEndReason expectedEndReason)
@@ -475,15 +474,13 @@ protected void CreateConnection()
475474
_pair = DuplexPipe.CreateConnectionPair(inputPipeOptions, outputPipeOptions);
476475

477476
_metricsTagsFeature = new TestConnectionMetricsTagsFeature();
478-
_errorCodeFeature = new TestProtocolErrorCodeFeature();
479477

480478
var metricsContext = TestContextFactory.CreateMetricsContext(_mockConnectionContext.Object);
481479
_metricsContextFeature = new TestConnectionMetricsContextFeature() { MetricsContext = metricsContext };
482480

483481
var features = new FeatureCollection();
484482
features.Set<IConnectionMetricsContextFeature>(_metricsContextFeature);
485483
features.Set<IConnectionMetricsTagsFeature>(_metricsTagsFeature);
486-
features.Set<IProtocolErrorCodeFeature>(_errorCodeFeature);
487484
_mockConnectionContext.Setup(x => x.Features).Returns(features);
488485
var httpConnectionContext = TestContextFactory.CreateHttpConnectionContext(
489486
serviceContext: _serviceContext,
@@ -515,11 +512,6 @@ private class TestConnectionMetricsContextFeature : IConnectionMetricsContextFea
515512
public ConnectionMetricsContext MetricsContext { get; init; }
516513
}
517514

518-
private class TestProtocolErrorCodeFeature : IProtocolErrorCodeFeature
519-
{
520-
public long Error { get; set; } = -1;
521-
}
522-
523515
private class LifetimeHandlerInterceptor : IHttp2StreamLifetimeHandler
524516
{
525517
private readonly IHttp2StreamLifetimeHandler _inner;

‎src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/TlsTests.cs

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -77,10 +77,7 @@ await sslStream.AuthenticateAsClientAsync(new SslClientAuthenticationOptions
7777
}
7878
}
7979

80-
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m =>
81-
{
82-
Assert.Equal(KestrelMetrics.GetErrorType(ConnectionEndReason.InsufficientTlsVersion), m.Tags[KestrelMetrics.ErrorType]);
83-
});
80+
Assert.Collection(connectionDuration.GetMeasurementSnapshot(), m => MetricsAssert.Equal(ConnectionEndReason.InsufficientTlsVersion, m.Tags));
8481
}
8582

8683
private async Task WaitForConnectionErrorAsync(PipeReader reader, bool ignoreNonGoAwayFrames, int expectedLastStreamId, Http2ErrorCode expectedErrorCode)

0 commit comments

Comments
 (0)