Skip to content

Commit 9e163d7

Browse files
joelverhagenjoelverhagen
committed
Don't track the package push failure if the client has disconnected (#8060)
Address #8022
1 parent d7385b8 commit 9e163d7

11 files changed

Lines changed: 151 additions & 1 deletion

File tree

‎src/GitHubVulnerabilities2Db/Gallery/ThrowingTelemetryService.cs‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,11 @@ public void TrackPackageOwnershipAutomaticallyAdded(string packageId, string pac
236236
throw new NotImplementedException();
237237
}
238238

239+
public void TrackPackagePushDisconnectEvent()
240+
{
241+
throw new NotImplementedException();
242+
}
243+
239244
public void TrackPackagePushEvent(Package package, User user, IIdentity identity)
240245
{
241246
throw new NotImplementedException();
@@ -316,6 +321,11 @@ public void TrackSymbolPackageFailedGalleryValidationEvent(string packageId, str
316321
throw new NotImplementedException();
317322
}
318323

324+
public void TrackSymbolPackagePushDisconnectEvent()
325+
{
326+
throw new NotImplementedException();
327+
}
328+
319329
public void TrackSymbolPackagePushEvent(string packageId, string packageVersion)
320330
{
321331
throw new NotImplementedException();

‎src/NuGetGallery.Services/Telemetry/ITelemetryService.cs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ public interface ITelemetryService
3232

3333
void TrackPackagePushFailureEvent(string id, NuGetVersion version);
3434

35+
void TrackPackagePushDisconnectEvent();
36+
3537
void TrackPackageUnlisted(Package package);
3638

3739
void TrackPackageListed(Package package);
@@ -202,6 +204,11 @@ void TrackPackageDeprecate(
202204
/// <param name="packageVersion">The version of the package that has the symbols uploaded.</param>
203205
void TrackSymbolPackagePushFailureEvent(string packageId, string packageVersion);
204206

207+
/// <summary>
208+
/// A telemetry event emitted when a symbol package push failed due to client disconnect.
209+
/// </summary>
210+
void TrackSymbolPackagePushDisconnectEvent();
211+
205212
/// <summary>
206213
/// A telemetry event emitted when a symbol package fails Gallery validation.
207214
/// </summary>

‎src/NuGetGallery.Services/Telemetry/TelemetryService.cs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,8 @@ public class Events
8787
public const string ABTestEnrollmentInitialized = "ABTestEnrollmentInitialized";
8888
public const string ABTestEnrollmentUpgraded = "ABTestEnrollmentUpgraded";
8989
public const string ABTestEvaluated = "ABTestEvaluated";
90+
public const string PackagePushDisconnect = "PackagePushDisconnect";
91+
public const string SymbolPackagePushDisconnect = "SymbolPackagePushDisconnect";
9092
}
9193

9294
private readonly IDiagnosticsSource _diagnosticsSource;
@@ -1082,6 +1084,16 @@ public void TrackABTestEvaluated(
10821084
});
10831085
}
10841086

1087+
public void TrackPackagePushDisconnectEvent()
1088+
{
1089+
TrackMetric(Events.PackagePushDisconnect, 1, p => { });
1090+
}
1091+
1092+
public void TrackSymbolPackagePushDisconnectEvent()
1093+
{
1094+
TrackMetric(Events.SymbolPackagePushDisconnect, 1, p => { });
1095+
}
1096+
10851097
/// <summary>
10861098
/// We use <see cref="ITelemetryClient.TrackMetric(string, double, IDictionary{string, string})"/> instead of
10871099
/// <see cref="ITelemetryClient.TrackEvent(string, IDictionary{string, string}, IDictionary{string, double})"/>

‎src/NuGetGallery/Controllers/ApiController.cs‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -512,6 +512,15 @@ await AuditingService.SaveAuditRecordAsync(
512512
HttpStatusCode.RequestEntityTooLarge,
513513
Strings.PackageFileTooLarge);
514514
}
515+
catch (HttpException ex) when (!Response.IsClientConnected)
516+
{
517+
// ASP.NET throws HttpException when the client has disconnected during the upload.
518+
TelemetryService.TrackSymbolPackagePushDisconnectEvent();
519+
QuietLog.LogHandledException(ex);
520+
return new HttpStatusCodeWithBodyResult(
521+
HttpStatusCode.BadRequest,
522+
Strings.PackageUploadCancelled);
523+
}
515524
catch (Exception ex)
516525
{
517526
ex.Log();
@@ -811,6 +820,15 @@ await AuditingService.SaveAuditRecordAsync(
811820
HttpStatusCode.RequestEntityTooLarge,
812821
Strings.PackageFileTooLarge);
813822
}
823+
catch (HttpException ex) when (!Response.IsClientConnected)
824+
{
825+
// ASP.NET throws HttpException when the client has disconnected during the upload.
826+
TelemetryService.TrackPackagePushDisconnectEvent();
827+
QuietLog.LogHandledException(ex);
828+
return new HttpStatusCodeWithBodyResult(
829+
HttpStatusCode.BadRequest,
830+
Strings.PackageUploadCancelled);
831+
}
814832
catch (Exception)
815833
{
816834
TelemetryService.TrackPackagePushFailureEvent(id, version);

‎src/NuGetGallery/Strings.Designer.cs‎

Lines changed: 9 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/NuGetGallery/Strings.resx‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1159,4 +1159,7 @@ The {1} Team</value>
11591159
<data name="TwoFAFeedback_Error" xml:space="preserve">
11601160
<value>There was an unexpected error when submitting feedback. Please contact NuGet support.</value>
11611161
</data>
1162+
<data name="PackageUploadCancelled" xml:space="preserve">
1163+
<value>The package upload failed due to the client disconnecting.</value>
1164+
</data>
11621165
</root>

‎src/NuGetGallery/Telemetry/CustomerResourceIdEnricher.cs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,10 @@ public class CustomerResourceIdEnricher : ITelemetryInitializer
2020
{
2121
TelemetryService.Events.PackagePush,
2222
TelemetryService.Events.PackagePushFailure,
23+
TelemetryService.Events.PackagePushDisconnect,
24+
TelemetryService.Events.SymbolPackagePush,
25+
TelemetryService.Events.SymbolPackagePushFailure,
26+
TelemetryService.Events.SymbolPackagePushDisconnect,
2327
};
2428

2529
public void Initialize(ITelemetry telemetry)

‎src/VerifyMicrosoftPackage/Fakes/FakeTelemetryService.cs‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,11 @@ public void TrackPackageOwnershipAutomaticallyAdded(string packageId, string pac
233233
throw new NotImplementedException();
234234
}
235235

236+
public void TrackPackagePushDisconnectEvent()
237+
{
238+
throw new NotImplementedException();
239+
}
240+
236241
public void TrackPackagePushEvent(Package package, User user, IIdentity identity)
237242
{
238243
throw new NotImplementedException();
@@ -313,6 +318,11 @@ public void TrackSymbolPackageFailedGalleryValidationEvent(string packageId, str
313318
throw new NotImplementedException();
314319
}
315320

321+
public void TrackSymbolPackagePushDisconnectEvent()
322+
{
323+
throw new NotImplementedException();
324+
}
325+
316326
public void TrackSymbolPackagePushEvent(string packageId, string packageVersion)
317327
{
318328
throw new NotImplementedException();

‎tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs‎

Lines changed: 66 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
using System.Linq;
1111
using System.Linq.Expressions;
1212
using System.Net;
13+
using System.Security.Principal;
1314
using System.Threading.Tasks;
1415
using System.Web;
1516
using System.Web.Mvc;
@@ -64,7 +65,8 @@ public TestableApiController(
6465
IGalleryConfigurationService configurationService,
6566
MockBehavior behavior = MockBehavior.Default,
6667
ISecurityPolicyService securityPolicyService = null,
67-
IUserService userService = null)
68+
IUserService userService = null,
69+
Mock<HttpResponseBase> responseMock = null)
6870
{
6971
SetOwinContextOverride(Fakes.CreateOwinContext());
7072
ApiScopeEvaluator = (MockApiScopeEvaluator = new Mock<IApiScopeEvaluator>()).Object;
@@ -159,6 +161,12 @@ public TestableApiController(
159161
requestMock.Setup(m => m.IsSecureConnection).Returns(true);
160162
requestMock.Setup(m => m.Url).Returns(new Uri(TestUtility.GallerySiteRootHttps));
161163

164+
if (responseMock == null)
165+
{
166+
responseMock = new Mock<HttpResponseBase>();
167+
responseMock.Setup(m => m.IsClientConnected).Returns(true);
168+
}
169+
162170
var httpContextMock = new Mock<HttpContextBase>();
163171
httpContextMock.Setup(m => m.Request).Returns(requestMock.Object);
164172

@@ -244,6 +252,33 @@ public async Task CreateSymbolPackage_ReturnsFailedActionWhenValidationFails()
244252
ResultAssert.IsStatusCode(result, HttpStatusCode.BadRequest);
245253
}
246254

255+
[Fact]
256+
public async Task CreateSymbolPackage_TracksClientDisconnectedWithoutFailureMetric()
257+
{
258+
// Arrange
259+
var responseMock = new Mock<HttpResponseBase>();
260+
responseMock.Setup(x => x.IsClientConnected).Returns(false);
261+
262+
var controller = new TestableApiController(GetConfigurationService(), responseMock: responseMock);
263+
controller.SetCurrentUser(new User() { EmailAddress = "[email protected]" });
264+
controller.MockSymbolPackageUploadService
265+
.Setup(p => p.ValidateUploadedSymbolsPackage(
266+
It.IsAny<Stream>(),
267+
It.IsAny<User>()))
268+
.Throws(new HttpException());
269+
270+
controller.SetupPackageFromInputStream(TestPackage.CreateTestSymbolPackageStream());
271+
272+
// Act
273+
var result = await controller.CreateSymbolPackagePutAsync();
274+
275+
// Assert
276+
ResultAssert.IsStatusCode(result, HttpStatusCode.BadRequest, "The package upload failed due to the client disconnecting.");
277+
controller.MockTelemetryService.Verify(x => x.TrackSymbolPackagePushDisconnectEvent(), Times.Once);
278+
controller.MockTelemetryService.Verify(x => x.TrackSymbolPackagePushEvent(It.IsAny<string>(), It.IsAny<string>()), Times.Never);
279+
controller.MockTelemetryService.Verify(x => x.TrackSymbolPackagePushFailureEvent(It.IsAny<string>(), It.IsAny<string>()), Times.Never);
280+
}
281+
247282
[Fact]
248283
public async Task CreateSymbolPackage_UnauthorizedUserWillGet403()
249284
{
@@ -456,6 +491,36 @@ public async Task CreatePackage_TracksFailureIfUnexpectedExceptionWithoutIdVersi
456491
controller.MockTelemetryService.Verify(x => x.TrackPackagePushFailureEvent(null, null), Times.Once());
457492
}
458493

494+
[Fact]
495+
public async Task CreatePackage_TracksClientDisconnectedWithoutFailureMetric()
496+
{
497+
// Arrange
498+
var responseMock = new Mock<HttpResponseBase>();
499+
responseMock.Setup(x => x.IsClientConnected).Returns(false);
500+
501+
var controller = new TestableApiController(GetConfigurationService(), responseMock: responseMock);
502+
controller.SetCurrentUser(new User() { EmailAddress = "[email protected]" });
503+
controller.MockPackageUploadService
504+
.Setup(p => p.GeneratePackageAsync(
505+
It.IsAny<string>(),
506+
It.IsAny<PackageArchiveReader>(),
507+
It.IsAny<PackageStreamMetadata>(),
508+
It.IsAny<User>(),
509+
It.IsAny<User>()))
510+
.Throws(new HttpException());
511+
512+
controller.SetupPackageFromInputStream(TestPackage.CreateTestPackageStream());
513+
514+
// Act
515+
var result = await controller.CreatePackagePut();
516+
517+
// Assert
518+
ResultAssert.IsStatusCode(result, HttpStatusCode.BadRequest, "The package upload failed due to the client disconnecting.");
519+
controller.MockTelemetryService.Verify(x => x.TrackPackagePushDisconnectEvent(), Times.Once);
520+
controller.MockTelemetryService.Verify(x => x.TrackPackagePushEvent(It.IsAny<Package>(), It.IsAny<User>(), It.IsAny<IIdentity>()), Times.Never);
521+
controller.MockTelemetryService.Verify(x => x.TrackPackagePushFailureEvent(It.IsAny<string>(), It.IsAny<NuGetVersion>()), Times.Never);
522+
}
523+
459524
[Fact]
460525
public async Task CreatePackage_TracksFailureIfUnexpectedExceptionWithIdVersion()
461526
{

‎tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -331,6 +331,14 @@ public static IEnumerable<object[]> TrackMetricNames_Data
331331
yield return new object[] { "ABTestEvaluated",
332332
(TrackAction)(s => s.TrackABTestEvaluated("SearchPreview", true, true, 0, 20))
333333
};
334+
335+
yield return new object[] { "PackagePushDisconnect",
336+
(TrackAction)(s => s.TrackPackagePushDisconnectEvent())
337+
};
338+
339+
yield return new object[] { "SymbolPackagePushDisconnect",
340+
(TrackAction)(s => s.TrackSymbolPackagePushDisconnectEvent())
341+
};
334342
}
335343
}
336344

0 commit comments

Comments
 (0)