Update PackageLagMonitor for AzureSearch #798

ryuyu · 2019-08-15T23:47:19Z

Adds support for specifying a ServiceType to configuration to allow monitoring of both CloudService based search and WebApp based search.
Updates telemetry dimensions so we can tell them apart.

…ceType through.

joelverhagen · 2019-08-15T23:47:54Z

src/PackageLagMonitor/AzureSearchDiagnosticResponse.cs

@@ -0,0 +1,75 @@
+using System;


copyright header

joelverhagen · 2019-08-15T23:48:45Z

src/PackageLagMonitor/AzureSearchDiagnosticResponse.cs

+    {
+        public string Name { get; set; }
+
+        public long DocumentCount { get; set; }


nit: only add the properties you use... the shape has been kind of fluid...

The LastCommitTimestamp can't change though since it's already used by a runner

joelverhagen · 2019-08-15T23:49:33Z

src/PackageLagMonitor/PackageLagCatalogLeafProcessor.cs

@@ -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);


You can pass the enum as-is. The ToString() will be called and result in this came thing

Cool. I'll do that instead.

joelverhagen · 2019-08-15T23:51:04Z

src/PackageLagMonitor/SearchServiceClient.cs

+                case ServiceType.AzureSearch:
+                    return GetInstances(endpointUri: new Uri(regionInformation.BaseUrl), instanceCount: 1, regionInformation: regionInformation, serviceType: ServiceType.AzureSearch);
+                default:
+                    throw new UnknownServiceTypeException();


You can throw an existing exception like NotImplementated with a message containing the service type to avoid the custom exception

joelverhagen · 2019-08-15T23:51:39Z

src/PackageLagMonitor/SearchServiceClient.cs


-            var instances = GetInstances(cloudService.Uri, cloudService.InstanceCount, regionInformation);
+                    var instances = GetInstances(endpointUri: cloudService.Uri, instanceCount: cloudService.InstanceCount, regionInformation: regionInformation, serviceType: ServiceType.LuceneSearch);


these named params are a bit redundant. the parameters themselves are very descriptive in this case

scottbommarito · 2019-08-15T23:55:38Z

src/PackageLagMonitor/SearchServiceClient.cs

-            return instances;
+                    return instances;
+                case ServiceType.AzureSearch:
+                    return GetInstances(endpointUri: new Uri(regionInformation.BaseUrl), instanceCount: 1, regionInformation: regionInformation, serviceType: ServiceType.AzureSearch);


I think the code would be a little simpler if you made it construct an instance here directly instead of calling GetInstances. GetInstances does a lot of work to create multiple instances that's not necessary for the Azure Search case.

I would recommend:

Moving the code inside the Enumerable.Range(...).Select(...) that generates an instance into its own function.

Replacing this call with a call to that function.

Renaming GetInstances to GetLuceneInstances.

I'm not sure what you mean here by simpler.
The switch for specifying port would still need to exist (as the new function would still need to be able to specify this), and the case for AzureSearch would have to manually wrap the return in a list (note the return type of the method).
I think it makes more sense to treat azure search instance as a "LuceneSearch" type with 1 instance (and admittedly no port) rather than trying to completely separate them.

I mean, it's a little confusing that GetInstances will always enumerate an enumerable with a single member in the AzureSearch case. The only thing that's really shared logic in that function is the construction of an instance (which, admittedly, has to check service type) so I was thinking you could move that out. Not a big deal though.

scottbommarito · 2019-08-16T00:27:43Z

src/PackageLagMonitor/PackageLagCatalogLeafProcessor.cs

-                    _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);


Shouldn't you be piping the instance.ServiceType to here and the other _telemetryService call?

Note the changes to TelemetryService already do this since we have access to the ServiceType on the instance.

Oh, I see now!

joelverhagen · 2019-08-16T15:41:42Z

src/PackageLagMonitor/UnknownServiceTypeException.cs

+
+namespace NuGet.Jobs.Monitoring.PackageLag
+{
+    public class UnknownServiceTypeException : Exception


joelverhagen · 2019-08-16T15:43:01Z

tests/Monitoring.RebootSearchInstance.Tests/SearchInstanceRebooterFacts.cs

@@ -77,19 +77,22 @@ public SearchInstanceRebooterFacts(ITestOutputHelper output)
                    0,
                    "http://localhost:801/search/diag",
                    "http://localhost:801/query",
-                    _region),
+                    _region,


We need some unit tests that somehow assert the flow of a) get /search/diag from Lucene -- use last re-open timestamp and b) get /search/diag from Azure Search -- use current time.

Whew ok. Added test for this.

joelverhagen · 2019-08-16T15:43:21Z

src/PackageLagMonitor/Telemetry/PackageLagTelemetryService.cs

@@ -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) }


you can just do instance.ServiceType.ToString()

joelverhagen

⌚️

joelverhagen · 2019-08-16T15:45:07Z

src/PackageLagMonitor/SearchServiceClient.cs

+        {
+            var result = new SearchDiagnosticResponse()
+            {
+                LastIndexReloadTime = DateTimeOffset.UtcNow,


We should link to that issue you filed about reducing the poll frequency. Maybe make a long-ish comment explaining the situation and how this introduces an error of up to poll frequency (if that is indeed the case).

Good call. I'll add a link here to the issue at least. And probably add a comment about why we do this.

loic-sharma · 2019-08-16T16:12:37Z

src/PackageLagMonitor/SearchServiceClient.cs

+
+        private SearchDiagnosticResponse ConvertAzureSearchResponse(AzureSearchDiagnosticResponse azureSearchDiagnosticResponse)
+        {
+            var result = new SearchDiagnosticResponse()


Nit: you can drop the () if you use object initializer syntax

Suggested change

var result = new SearchDiagnosticResponse()

var result = new SearchDiagnosticResponse

Same comment for line 240

scottbommarito

Add unit tests as @joelverhagen requested and then ship it!

joelverhagen · 2019-08-19T20:04:55Z

tests/Monitoring.PackageLag.Tests/SearchServiceClientFacts.cs

+                }))));
+            var searchClient = new SearchServiceClient(_azureApiWrapper, httpClientMock.Object, _options, _logger);
+
+            var luceneResponse = searchClient.GetIndexLastReloadTimeAsync(_luceneInstance, token).Result;


this test can be made async

joelverhagen · 2019-08-19T20:05:32Z

tests/Monitoring.PackageLag.Tests/SearchServiceClientFacts.cs

+            var luceneResponse = searchClient.GetIndexLastReloadTimeAsync(_luceneInstance, token).Result;
+            var azureResponse = searchClient.GetIndexLastReloadTimeAsync(_azureSearchInstance, token).Result;
+
+            Assert.True(azureResponse - DateTimeOffset.UtcNow < new TimeSpan(hours: 0, minutes: 0, seconds: 10));


would be better to capture datetimeoffset.utcnow before and after GetIndexLastReloadTimeAsync then use Assert.InRange

joelverhagen · 2019-08-19T20:06:22Z

tests/Monitoring.PackageLag.Tests/SearchServiceClientFacts.cs

+            var azureResponse = searchClient.GetIndexLastReloadTimeAsync(_azureSearchInstance, token).Result;
+
+            Assert.True(azureResponse - DateTimeOffset.UtcNow < new TimeSpan(hours: 0, minutes: 0, seconds: 10));
+            Assert.True(luceneResponse.Equals(new DateTimeOffset()));


Assert.Equals

Maybe use an explicit timestamp that is not default(DateTimeOffset) since this could coincidentally be right instead of do to the expected impl

joelverhagen

Address comments then

* Update instance generation to understand azureSearch type. Pipe serviceType through. * Support AzureSearchDiagResponse. Pipe ServiceType through to telemetry. * Fix tests. * Fixed committimestamp to be pessimistic. * Add test for commit time stamp. PR feedback. * Async test.

ryuyu added 3 commits August 15, 2019 14:19

Update instance generation to understand azureSearch type. Pipe servi…

bf4387b

…ceType through.

Support AzureSearchDiagResponse. Pipe ServiceType through to telemetry.

09bc4a4

Fix tests.

ab3122d

ryuyu requested review from joelverhagen, agr, loic-sharma, xavierdecoster, shishirx34, cristinamanum, skofman1, dannyjdev, scottbommarito and zhhyu August 15, 2019 23:47

joelverhagen reviewed Aug 15, 2019

View reviewed changes

scottbommarito reviewed Aug 16, 2019

View reviewed changes

Fixed committimestamp to be pessimistic.

359dd35

joelverhagen reviewed Aug 16, 2019

View reviewed changes

joelverhagen suggested changes Aug 16, 2019

View reviewed changes

joelverhagen reviewed Aug 16, 2019

View reviewed changes

loic-sharma reviewed Aug 16, 2019

View reviewed changes

scottbommarito approved these changes Aug 16, 2019

View reviewed changes

Add test for commit time stamp. PR feedback.

e966158

joelverhagen reviewed Aug 19, 2019

View reviewed changes

joelverhagen approved these changes Aug 19, 2019

View reviewed changes

Async test.

9e833ce

ryuyu merged commit b25b7d9 into dev Aug 21, 2019

joelverhagen pushed a commit that referenced this pull request Jul 31, 2020

Propagate vulnerability info to registration blobs (#798)

6f93d9c

joelverhagen pushed a commit that referenced this pull request Oct 26, 2020

Propagate vulnerability info to registration blobs (#798)

c8298cd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update PackageLagMonitor for AzureSearch #798

Update PackageLagMonitor for AzureSearch #798

ryuyu commented Aug 15, 2019

joelverhagen Aug 15, 2019

ryuyu Aug 16, 2019

joelverhagen Aug 15, 2019

joelverhagen Aug 15, 2019

ryuyu Aug 16, 2019

joelverhagen Aug 15, 2019

joelverhagen Aug 15, 2019

ryuyu Aug 16, 2019

scottbommarito Aug 15, 2019

ryuyu Aug 16, 2019

scottbommarito Aug 16, 2019

scottbommarito Aug 16, 2019

ryuyu Aug 16, 2019

scottbommarito Aug 16, 2019

joelverhagen Aug 16, 2019

ryuyu Aug 16, 2019

joelverhagen Aug 16, 2019

ryuyu Aug 16, 2019

joelverhagen Aug 16, 2019

ryuyu Aug 16, 2019

joelverhagen left a comment

joelverhagen Aug 16, 2019

ryuyu Aug 16, 2019

loic-sharma Aug 16, 2019

ryuyu Aug 16, 2019

scottbommarito left a comment

joelverhagen Aug 19, 2019

ryuyu Aug 21, 2019

joelverhagen Aug 19, 2019

ryuyu Aug 21, 2019

joelverhagen Aug 19, 2019

ryuyu Aug 21, 2019

joelverhagen left a comment


		var instances = GetInstances(cloudService.Uri, cloudService.InstanceCount, regionInformation);
		var instances = GetInstances(endpointUri: cloudService.Uri, instanceCount: cloudService.InstanceCount, regionInformation: regionInformation, serviceType: ServiceType.LuceneSearch);

	var result = new SearchDiagnosticResponse()
	var result = new SearchDiagnosticResponse

Update PackageLagMonitor for AzureSearch #798

Update PackageLagMonitor for AzureSearch #798

Conversation

ryuyu commented Aug 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelverhagen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottbommarito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelverhagen left a comment

Choose a reason for hiding this comment