-
Notifications
You must be signed in to change notification settings - Fork 21
Update PackageLagMonitor for AzureSearch #798
Changes from 3 commits
bf4387b
09bc4a4
ab3122d
359dd35
e966158
9e833ce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
using System; | ||
|
||
namespace NuGet.Jobs.Monitoring.PackageLag | ||
{ | ||
public class AzureSearchDiagnosticResponse | ||
{ | ||
public bool Success { get; set; } | ||
|
||
public TimeSpan Duration { get; set; } | ||
|
||
public Server Server { get; set; } | ||
|
||
public IndexInformation SearchIndex { get; set; } | ||
|
||
public IndexInformation HijackIndex { get; set; } | ||
|
||
public AuxiliaryFileInformations AuxiliaryFiles { get; set; } | ||
} | ||
|
||
public class Server | ||
{ | ||
public string MachineName { get; set; } | ||
|
||
public long ProcessId { get; set; } | ||
|
||
public DateTimeOffset ProcessStartTime { get; set; } | ||
|
||
public TimeSpan ProcessDuration { get; set; } | ||
|
||
public string DeploymentLabel { get; set; } | ||
|
||
public string AssemblyCommitId { get; set; } | ||
|
||
public string AssemblyInformationVersion { get; set; } | ||
|
||
public DateTimeOffset AssemblyBuildDateUtc { get; set; } | ||
|
||
public string InstanceId { get; set; } | ||
} | ||
|
||
public class IndexInformation | ||
{ | ||
public string Name { get; set; } | ||
|
||
public long DocumentCount { get; set; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: only add the properties you use... the shape has been kind of fluid... The |
||
|
||
public TimeSpan DocumentCountDuration { get; set; } | ||
|
||
public TimeSpan WarmQueryDuration { get; set; } | ||
|
||
public DateTimeOffset LastCommitTimestamp { get; set; } | ||
|
||
public TimeSpan LastCommitTimestampDuration { get; set; } | ||
} | ||
|
||
public class AuxiliaryFileInformations | ||
{ | ||
public AuxiliaryFileInformation Downloads { get; set; } | ||
|
||
public AuxiliaryFileInformation VerifiedPackages { get; set; } | ||
} | ||
|
||
public class AuxiliaryFileInformation | ||
{ | ||
public DateTimeOffset LastModified { get; set; } | ||
|
||
public DateTimeOffset Loaded { get; set; } | ||
|
||
public TimeSpan LoadDuration { get; set; } | ||
|
||
public long FileSize { get; set; } | ||
|
||
public string ETag { get; set; } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,11 +4,9 @@ | |
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Net.Http; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Microsoft.Extensions.Logging; | ||
using Newtonsoft.Json; | ||
using NuGet.Jobs.Monitoring.PackageLag.Telemetry; | ||
using NuGet.Protocol.Catalog; | ||
|
||
|
@@ -132,6 +130,7 @@ public Task<bool> ProcessPackageDetailsAsync(PackageDetailsCatalogLeaf leaf) | |
CancellationToken token) | ||
{ | ||
await Task.Yield(); | ||
var serviceTypeString = Enum.GetName(typeof(ServiceType), instance.ServiceType); | ||
|
||
try | ||
{ | ||
|
@@ -171,7 +170,7 @@ public Task<bool> ProcessPackageDetailsAsync(PackageDetailsCatalogLeaf leaf) | |
if (shouldRetry) | ||
{ | ||
++retryCount; | ||
_logger.LogInformation("Waiting for {RetryTime} seconds before retrying {PackageId} {PackageVersion} against {SearchBaseUrl}", WaitBetweenPolls.TotalSeconds, packageId, packageVersion, instance.BaseQueryUrl); | ||
_logger.LogInformation("{ServiceType}: Waiting for {RetryTime} seconds before retrying {PackageId} {PackageVersion} against {SearchBaseUrl}", serviceTypeString, WaitBetweenPolls.TotalSeconds, packageId, packageVersion, instance.BaseQueryUrl); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can pass the enum as-is. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool. I'll do that instead. |
||
await Task.Delay(WaitBetweenPolls); | ||
} | ||
} while (shouldRetry && retryCount < RetryLimit); | ||
|
@@ -187,8 +186,8 @@ public Task<bool> ProcessPackageDetailsAsync(PackageDetailsCatalogLeaf leaf) | |
var timeStamp = (isListOperation ? lastEdited : created); | ||
|
||
// We log both of these values here as they will differ if a package went through validation pipline. | ||
_logger.LogInformation("Lag {Timestamp}:{PackageId} {PackageVersion} SearchInstance:{Region}{Instance} Created: {CreatedLag} V3: {V3Lag}", timeStamp, packageId, packageVersion, instance.Region, instance.Index, createdDelay, v3Delay); | ||
_logger.LogInformation("LastReload:{LastReloadTimestamp} LastEdited:{LastEditedTimestamp} Created:{CreatedTimestamp} ", lastReloadTime, lastEdited, created); | ||
_logger.LogInformation("{ServiceType}: Lag {Timestamp}:{PackageId} {PackageVersion} SearchInstance:{Region}{Instance} Created: {CreatedLag} V3: {V3Lag}", serviceTypeString, timeStamp, packageId, packageVersion, instance.Region, instance.Index, createdDelay, v3Delay); | ||
_logger.LogInformation("{ServiceType}: LastReload:{LastReloadTimestamp} LastEdited:{LastEditedTimestamp} Created:{CreatedTimestamp} ", serviceTypeString, lastReloadTime, lastEdited, created); | ||
if (!isListOperation) | ||
{ | ||
_telemetryService.TrackPackageCreationLag(timeStamp, instance, packageId, packageVersion, createdDelay); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't you be piping the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note the changes to TelemetryService already do this since we have access to the ServiceType on the instance. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I see now! |
||
|
@@ -204,12 +203,12 @@ public Task<bool> ProcessPackageDetailsAsync(PackageDetailsCatalogLeaf leaf) | |
} | ||
else | ||
{ | ||
_logger.LogInformation("Lag check for {PackageId} {PackageVersion} was abandoned. Retry limit reached.", packageId, packageVersion); | ||
_logger.LogInformation("{ServiceType}: Lag check for {PackageId} {PackageVersion} was abandoned. Retry limit reached.", serviceTypeString, packageId, packageVersion); | ||
} | ||
} | ||
catch (Exception e) | ||
{ | ||
_logger.LogError("Failed to compute lag for {PackageId} {PackageVersion}. {Exception}", packageId, packageVersion, e); | ||
_logger.LogError("{ServiceType}: Failed to compute lag for {PackageId} {PackageVersion}. {Exception}", serviceTypeString, packageId, packageVersion, e); | ||
} | ||
|
||
return null; | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -86,7 +86,17 @@ public async Task<SearchDiagnosticResponse> GetSearchDiagnosticResponseAsync( | |||||
|
||||||
var diagContent = diagResponse.Content; | ||||||
var searchDiagResultRaw = await diagContent.ReadAsStringAsync(); | ||||||
var response = JsonConvert.DeserializeObject<SearchDiagnosticResponse>(searchDiagResultRaw); | ||||||
SearchDiagnosticResponse response = null; | ||||||
switch (instance.ServiceType) | ||||||
{ | ||||||
case ServiceType.LuceneSearch: | ||||||
response = JsonConvert.DeserializeObject<SearchDiagnosticResponse>(searchDiagResultRaw); | ||||||
break; | ||||||
case ServiceType.AzureSearch: | ||||||
var tempResponse = JsonConvert.DeserializeObject<AzureSearchDiagnosticResponse>(searchDiagResultRaw); | ||||||
response = ConvertAzureSearchResponse(tempResponse); | ||||||
break; | ||||||
} | ||||||
|
||||||
return response; | ||||||
} | ||||||
|
@@ -107,18 +117,26 @@ public async Task<IReadOnlyList<Instance>> GetSearchEndpointsAsync( | |||||
RegionInformation regionInformation, | ||||||
CancellationToken token) | ||||||
{ | ||||||
var result = await _azureManagementApiWrapper.GetCloudServicePropertiesAsync( | ||||||
_configuration.Value.Subscription, | ||||||
regionInformation.ResourceGroup, | ||||||
regionInformation.ServiceName, | ||||||
ProductionSlot, | ||||||
token); | ||||||
switch (regionInformation.ServiceType) | ||||||
{ | ||||||
case ServiceType.LuceneSearch: | ||||||
var result = await _azureManagementApiWrapper.GetCloudServicePropertiesAsync( | ||||||
_configuration.Value.Subscription, | ||||||
regionInformation.ResourceGroup, | ||||||
regionInformation.ServiceName, | ||||||
ProductionSlot, | ||||||
token); | ||||||
|
||||||
var cloudService = AzureHelper.ParseCloudServiceProperties(result); | ||||||
var cloudService = AzureHelper.ParseCloudServiceProperties(result); | ||||||
|
||||||
var instances = GetInstances(cloudService.Uri, cloudService.InstanceCount, regionInformation); | ||||||
var instances = GetInstances(endpointUri: cloudService.Uri, instanceCount: cloudService.InstanceCount, regionInformation: regionInformation, serviceType: ServiceType.LuceneSearch); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these named params are a bit redundant. the parameters themselves are very descriptive in this case There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||||||
|
||||||
return instances; | ||||||
return instances; | ||||||
case ServiceType.AzureSearch: | ||||||
return GetInstances(endpointUri: new Uri(regionInformation.BaseUrl), instanceCount: 1, regionInformation: regionInformation, serviceType: ServiceType.AzureSearch); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the code would be a little simpler if you made it construct an instance here directly instead of calling I would recommend:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what you mean here by simpler. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean, it's a little confusing that |
||||||
default: | ||||||
throw new UnknownServiceTypeException(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can throw an existing exception like |
||||||
} | ||||||
} | ||||||
|
||||||
public async Task<SearchResultResponse> GetSearchResultAsync(Instance instance, string query, CancellationToken token) | ||||||
|
@@ -160,15 +178,26 @@ public async Task<SearchResultResponse> GetSearchResultAsync(Instance instance, | |||||
throw new NotImplementedException(); | ||||||
} | ||||||
|
||||||
private List<Instance> GetInstances(Uri endpointUri, int instanceCount, RegionInformation regionInformation) | ||||||
private List<Instance> GetInstances(Uri endpointUri, int instanceCount, RegionInformation regionInformation, ServiceType serviceType) | ||||||
{ | ||||||
var instancePortMinimum = _configuration.Value.InstancePortMinimum; | ||||||
|
||||||
_logger.LogInformation( | ||||||
"Testing {InstanceCount} instances, starting at port {InstancePortMinimum} for region {Region}.", | ||||||
instanceCount, | ||||||
instancePortMinimum, | ||||||
regionInformation.Region); | ||||||
switch (serviceType) | ||||||
{ | ||||||
case ServiceType.LuceneSearch: | ||||||
_logger.LogInformation( | ||||||
"{ServiceType}: Testing {InstanceCount} instances, starting at port {InstancePortMinimum} for region {Region}.", | ||||||
Enum.GetName(typeof(ServiceType), ServiceType.LuceneSearch), | ||||||
instanceCount, | ||||||
instancePortMinimum, | ||||||
regionInformation.Region); | ||||||
break; | ||||||
case ServiceType.AzureSearch: | ||||||
_logger.LogInformation( | ||||||
"{ServiceType}: Testing for region {Region}.", | ||||||
Enum.GetName(typeof(ServiceType), ServiceType.AzureSearch), | ||||||
regionInformation.Region); | ||||||
break; | ||||||
} | ||||||
|
||||||
return Enumerable | ||||||
.Range(0, instanceCount) | ||||||
|
@@ -177,23 +206,48 @@ private List<Instance> GetInstances(Uri endpointUri, int instanceCount, RegionIn | |||||
var diagUriBuilder = new UriBuilder(endpointUri); | ||||||
|
||||||
diagUriBuilder.Scheme = "https"; | ||||||
diagUriBuilder.Port = instancePortMinimum + i; | ||||||
if (serviceType == ServiceType.LuceneSearch) | ||||||
{ | ||||||
diagUriBuilder.Port = instancePortMinimum + i; | ||||||
} | ||||||
diagUriBuilder.Path = "search/diag"; | ||||||
|
||||||
var queryBaseUriBuilder = new UriBuilder(endpointUri); | ||||||
|
||||||
queryBaseUriBuilder.Scheme = "https"; | ||||||
queryBaseUriBuilder.Port = instancePortMinimum + i; | ||||||
if (serviceType == ServiceType.LuceneSearch) | ||||||
{ | ||||||
queryBaseUriBuilder.Port = instancePortMinimum + i; | ||||||
} | ||||||
queryBaseUriBuilder.Path = "search/query"; | ||||||
|
||||||
return new Instance( | ||||||
ProductionSlot, | ||||||
i, | ||||||
diagUriBuilder.Uri.ToString(), | ||||||
queryBaseUriBuilder.Uri.ToString(), | ||||||
regionInformation.Region); | ||||||
regionInformation.Region, | ||||||
serviceType); | ||||||
}) | ||||||
.ToList(); | ||||||
} | ||||||
|
||||||
private SearchDiagnosticResponse ConvertAzureSearchResponse(AzureSearchDiagnosticResponse azureSearchDiagnosticResponse) | ||||||
{ | ||||||
var result = new SearchDiagnosticResponse() | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: you can drop the
Suggested change
Same comment for line 240 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||||||
{ | ||||||
NumDocs = azureSearchDiagnosticResponse.SearchIndex.DocumentCount, | ||||||
IndexName = azureSearchDiagnosticResponse.SearchIndex.Name, | ||||||
LastIndexReloadTime = azureSearchDiagnosticResponse.SearchIndex.LastCommitTimestamp, | ||||||
LastIndexReloadDurationInMilliseconds = (long)azureSearchDiagnosticResponse.SearchIndex.LastCommitTimestampDuration.TotalMilliseconds, | ||||||
LastReopen = azureSearchDiagnosticResponse.SearchIndex.LastCommitTimestamp, | ||||||
CommitUserData = new CommitUserData() | ||||||
{ | ||||||
CommitTimeStamp = azureSearchDiagnosticResponse.SearchIndex.LastCommitTimestamp.ToString() | ||||||
} | ||||||
}; | ||||||
|
||||||
return result; | ||||||
} | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
namespace NuGet.Jobs.Monitoring.PackageLag | ||
{ | ||
public enum ServiceType | ||
{ | ||
LuceneSearch, | ||
AzureSearch | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ public class PackageLagTelemetryService : IPackageLagTelemetryService | |
private const string Region = "Region"; | ||
private const string Subscription = "Subscription"; | ||
private const string InstanceIndex = "InstanceIndex"; | ||
private const string ServiceType = "ServiceType"; | ||
|
||
private const string CreatedLagName = "PackageCreationLagInSeconds"; | ||
private const string V3LagName = "V3LagInSeconds"; | ||
|
@@ -36,7 +37,8 @@ public void TrackPackageCreationLag(DateTimeOffset eventTime, Instance instance, | |
{ PackageVersion, packageVersion }, | ||
{ Region, instance.Region }, | ||
{ Subscription, _subscription }, | ||
{ InstanceIndex, instance.Index.ToString() } | ||
{ InstanceIndex, instance.Index.ToString() }, | ||
{ ServiceType, Enum.GetName(typeof(ServiceType), instance.ServiceType) } | ||
}); | ||
} | ||
|
||
|
@@ -48,7 +50,8 @@ public void TrackV3Lag(DateTimeOffset eventTime, Instance instance, string packa | |
{ PackageVersion, packageVersion }, | ||
{ Region, instance.Region }, | ||
{ Subscription, _subscription }, | ||
{ InstanceIndex, instance.Index.ToString() } | ||
{ InstanceIndex, instance.Index.ToString() }, | ||
{ ServiceType, Enum.GetName(typeof(ServiceType), instance.ServiceType) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can just do instance.ServiceType.ToString() There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updasted |
||
}); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
|
||
namespace NuGet.Jobs.Monitoring.PackageLag | ||
{ | ||
public class UnknownServiceTypeException : Exception | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unused now There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed. |
||
{ | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copyright header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added