Status aggregator job #494

scottbommarito · 2018-07-25T22:15:21Z

This job aggregates incidents and exports them in a blob that can be consumed by other services.

It maintains the state of every incident and groups incidents that affect the same component at the same time and reports them as an event.

Incidents are parsed through their titles. We discard incidents that we can't parse, and maintain a cursor that described what incidents have been processed.

The job will create up to two messages for each event. If all incidents attached to an event are mitigated within a short period of time, the job will never create any messages for an event. Otherwise, the job will create a message for the start of an event and the end of an event.

See https://nuget-dev-0-status.azurewebsites.net/ to see an example of how the data exported by this job will be used.

loic-sharma · 2018-07-25T23:55:03Z

src/StatusAggregator/Job.cs

+        private const int _defaultEventEndDelayMinutes = 10;
+        private const int _defaultEventVisibilityPeriod = 10;
+
+        private static void AddConfiguration(IServiceCollection serviceCollection, IDictionary<string, string> jobArgsDictionary)


AddConfiguration [](start = 28, length = 16)

Any chance we could use json files like in the validation jobs? There's quite a lot of arguments here

I'm not super familiar with how the JSON configuration works, but I can check it out.

loic-sharma · 2018-07-25T23:57:00Z

src/StatusAggregator/Program.cs

+        public static void Main(string[] args)
+        {
+            var job = new Job();
+            JobRunner.Run(job, args).Wait();


Wait [](start = 37, length = 4)

Wait() should be GetAwaiter().GetResult()

We should do this for our other jobs as well. Filed NuGet/NuGetGallery#6224

loic-sharma · 2018-07-26T00:04:30Z

src/StatusAggregator/IncidentUpdater.cs

+                ? ""
+                : $" and CreateDate gt datetime'{cursor.ToString("o")}'";
+
+            return $"$filter=OwningTeamId eq '{_incidentApiTeamId}'{cursorPart}";


{cursorPart} [](start = 67, length = 12)

This might read a little bit better if you flip this around. if cursorPart != DateTime.MinValue, append to the query string the CreateDate bit. Feel free to ignore :)

private string GetRecentIncidentsQuery(DateTime cursor) { var query = $"$filter = OwningTeamId eq '{_incidentApiTeamId}'"; if (cursor != DateTime.MinValue) { query += $" and CreateDate gt datetime'{cursor.ToString("o")}'"; } return query; }

Looks much better, good catch!

loic-sharma · 2018-07-26T00:07:03Z

src/StatusAggregator/Parse/IncidentParser.cs

+            {
+                parsedIncident = null;
+                var title = incident.Title;
+                var match = Regex.Match(title, _regExPattern);


Match [](start = 34, length = 5)

I forget all the requirements around Regex, but, I know there's a timeout one. I would double check what's necessary to run regex.

Looked at other uses of Regex in our code, it appears the requirement is that we add a timeout. Will add a timeout of 5 seconds.

loic-sharma · 2018-07-27T20:42:15Z

src/NuGet.Jobs.Common/Extensions/ServiceProviderExtensions.cs

+{
+    public static class ServiceProviderExtensions
+    {
+        public static T GetService<T>(this IServiceProvider provider)


GetService [](start = 24, length = 10)

This already exists: GetRequiredService<T>

Removed and replaced the uses of this new extension

loic-sharma · 2018-07-27T20:42:33Z

src/StatusAggregator/Job.cs

+        public override Task Run()
+        {
+            return _serviceProvider
+                .GetService<StatusAggregator>()


GetService [](start = 17, length = 10)

You can use GetRequiredService<StatusAggregator> here.

loic-sharma · 2018-07-27T20:45:33Z

src/StatusAggregator/Cursor.cs

+            ITableWrapper table,
+            ILogger<Cursor> logger)
+        {
+            _table = table;


table [](start = 21, length = 5)

I would add null checks to all constructors. From my experience, it makes catching null reference exceptions easier :)

loic-sharma · 2018-07-27T20:45:44Z

src/StatusAggregator/Cursor.cs

+using StatusAggregator.Table;
+
+
+namespace StatusAggregator


StatusAggregator [](start = 10, length = 16)

There's two empty lines above this one

good catch!

loic-sharma · 2018-07-27T20:48:18Z

src/StatusAggregator/Cursor.cs

+                {
+                    value = DateTime.MinValue;
+                    _logger.LogInformation("Could not fetch cursor.");
+                }


It's not super clear from this log that the cursor will be set to the beginning of time. Maybe log the value here? Feel free to ignore

I'll add that to the log

loic-sharma · 2018-07-27T20:58:47Z

src/StatusAggregator/EventUpdater.cs

+                var shouldDeactivate = !incidentsLinkedToEventQuery
+                        .Where(i => i.IsActive || i.MitigationTime > cursor - _eventEndDelay)
+                        .ToList()
+                        .Any();


This ToList() forces all incidents to be inspected. This LINQ query will do less work if you remove the ToList() and do just Any().

The IQueryable returned by _table.CreateQuery doesn't support Any, so I have to do ToList.

Ah gotcha :(

In reply to: 205907243 [](ancestors = 205907243)

loic-sharma · 2018-07-27T21:24:54Z

src/StatusAggregator/MessageUpdater.cs

+            using (_logger.Scope("Creating message for start of event."))
+            {
+                if (cursor > eventEntity.StartTime + _eventStartMessageDelay)
+                {


I think this would be easier to read if you invert the "error" flow:

if (!condition1) { _logger.LogInformation("Reason 1 for not doing anything"); return; } if (!condition2) { _logger.LogInformation("Reason 2 for not doing anything"); return; }

good catch!

loic-sharma · 2018-07-27T22:17:08Z

src/StatusAggregator/MessageUpdater.cs

+
+            var componentNames = path.Split(Constants.ComponentPathDivider);
+            var name = string.Join(" ", componentNames.Skip(1).Reverse());
+            var boldedPart = $"<b>{name} {boldedPartInnerString} {_componentStatusNames[eventEntity.AffectedComponentStatus].ToLowerInvariant()}.</b>";


_componentStatusNames[eventEntity.AffectedComponentStatus].ToLowerInvariant() [](start = 66, length = 77)

Why not eventEntity.AffectedComponentStatus.ToString().ToLowerInvariant()?

The current code will break if we ever modify ComponentStatus to have values that aren't sequential (ex: Up = 0, Down = 1, Degraded = 3).

loic-sharma · 2018-07-27T22:24:07Z

src/StatusAggregator/MessageUpdater.cs

+            }
+        }
+
+        private bool TryGetContentsForEventStartForEvent(EventEntity eventEntity, out string contents)


TryGetContentsForEventStartForEvent [](start = 21, length = 35)

This code is hard to understand. Can we simplify it?

Some ideas:

Get rid of variables like _youMayEncounterIssues and just place the text directly into the message. It's fine to have duplication.

Instead of passing parameters like boldedPartInnerString and suffix, pass a message template. Something like <b>{ComponentName} is {ComponentStatus}</b>. {EventMessage}. and <b>{ComponentName} is no longer {ComponentStatus}</b>. {EventMessage}. Thank you for your patience.

Done.

But this makes the package publishing degraded message less informative, unfortunately.

cristinamanum

Please review the comments and decide if what makes sense to update.

cristinamanum · 2018-07-27T22:16:15Z

src/NuGet.Jobs.Common/Configuration/JobConfigurationManager.cs

@@ -126,6 +127,13 @@ public static bool TryGetBoolArgument(IDictionary<string, string> jobArgsDiction
            return null;
        }

+        public static T TryGetEnumArgument<T>(IDictionary<string, string> jobArgsDictionary, string argName, T defaultValue)


Is this method used anywhere?

I added it initially, must have removed it. Great catch!

cristinamanum · 2018-07-27T22:18:29Z

src/NuGet.Jobs.Common/Extensions/LoggerExtensions.cs

+
+namespace NuGet.Jobs.Extensions
+{
+    public static class LoggerExtensions


Please add comments. Why the current login is not enough? Why this method should be used over other methods?

BeginScope on its own doesn't log a message when entering and leaving scope. This method does.

Will add comments.

cristinamanum · 2018-07-27T22:21:00Z

src/StatusAggregator/ComponentFactory.cs

+
+namespace StatusAggregator
+{
+    public static class ComponentFactory


Please add comments. What is a Component?

NuGet/ServerCommon#170 for more context. I agree that this class should have more comments however.

cristinamanum · 2018-07-27T22:25:24Z

src/StatusAggregator/Cursor.cs

+            using (_logger.Scope("Fetching cursor."))
+            {
+                var cursor = await _table.Retrieve<CursorEntity>(
+                    CursorEntity.DefaultPartitionKey, CursorEntity.DefaultRowKey);


Is it ok to get a null cursor back? Should it throw instead of setting to minvalue?

If there's no cursor, it likely means the table is empty and this is the first time the job has been run. So it should start at the beginning of time.

I'll add a comment to make this behavior clearer.

cristinamanum · 2018-07-27T22:27:53Z

src/StatusAggregator/Cursor.cs

+            using (_logger.Scope("Updating cursor to {Cursor}.", value))
+            {
+                var cursorEntity = new CursorEntity(value);
+                return _table.InsertOrReplaceAsync(cursorEntity);


Is this table thread safe? Does it need to be thread safe?

Please add comments to this either way.

It's Azure table storage, which is thread safe.

cristinamanum · 2018-07-27T22:44:14Z

src/StatusAggregator/IncidentUpdater.cs

+
+                var parsedIncidents = incidents
+                    .SelectMany(i => _aggregateIncidentParser.ParseIncident(i))
+                    .ToList();


Why not include orderby here and not do the ToList?

cristinamanum · 2018-07-27T22:45:32Z

src/StatusAggregator/IncidentUpdater.cs

+            }
+        }
+
+        private string GetRecentIncidentsQuery(DateTime cursor)


This is private method. Can it be tested?

The idea is that this can be tested as a part of tests for FetchNewIncidents because all this method does is create an argument for an _incidentApiClient call that can be mocked.

cristinamanum · 2018-07-27T23:39:08Z

src/StatusAggregator/MessageUpdater.cs

+
+        private static string[] _componentStatusNames = Enum.GetNames(typeof(ComponentStatus));
+        private bool TryGetContentsForEventHelper(
+            EventEntity eventEntity, 


Is this method testable? Will it be better to have it public for easier testing?

I don't think there would be any benefit unless we abstracted this out to a different class. We still need to test that the other methods that call this method do what this method does.

cristinamanum · 2018-07-27T23:42:16Z

src/StatusAggregator/Parse/IncidentParser.cs

+            string regExPattern, 
+            ILogger<IncidentParser> logger)
+        {
+            _regExPattern = regExPattern;


null checks

cristinamanum · 2018-07-27T23:48:53Z

src/StatusAggregator/Parse/ValidationDurationIncidentParser.cs

+{
+    public class ValidationDurationIncidentParser : EnvironmentPrefixIncidentParser
+    {
+        private const string SubtitleRegEx = "Too many packages are stuck in the \"Validating\" state!";


What happens if we change the incident title ? Do we have any way to monitor when that will happen?

Any change there we will need to rebuild and redeploy. Would it make sense to put it in a config?

We shouldn't be changing our incident titles. They should be descriptive enough on their own. If they aren't descriptive enough, we should change them to be more descriptive so that there isn't any desire to change them. Additionally, the job only needs to parse the title on ingestion, so if the name is changed after the job reads the incident (which should be within the first minute or two after it's created) it's fine.

Jobs deployments are generally pretty quick, so if the incident titles are changed (which should be infrequently), I don't think it would be a big deal to redeploy it.

loic-sharma · 2018-07-30T17:47:43Z

src/StatusAggregator/Parse/IncidentParser.cs

+        public bool TryParseIncident(Incident incident, out ParsedIncident parsedIncident)
+        {
+            using (_logger.Scope("Parsing incident with parser {IncidentParserType} using {RegExPattern}", 
+                GetType(), _regExPattern))


I think this log line would be a better place to log the incident's title than line 71. Something like: Attempting to use parser {IncidentParserType} using {RegExPattern} to parse title {IncidentTitle}.

agreed. done

Scott Bommarito added 30 commits June 27, 2018 12:36

doesn't make messages yet

e4d0f86

Merge branch 'dev' of https://github.com/NuGet/NuGet.Jobs into sb-icm

b318dbe

add messaging

03c1478

export simple blob

d27b115

reorganized status classes

d828566

refactor classes add test project

822f756

remove servercommon classes, add some tests

7a96990

fixed bugs and configured deployment

fd8d8ce

add statusaggregator to build scripts

0eaf2ec

sign status aggregator

04dbf01

fix containername in script

8b5e98f

add missing caret

69dab67

get certificate from keyvault

cb04d65

exporter fixes

ceea509

disable flaky test

8de4d99

fix messagings and export logic

ec75f37

"thank you for your patience"

aaadf93

revert applicationhost.config changes

a13521d

integrate with pingdom

71e6ea4

add messaging for other types of events

c60ffbf

check that event has no messages before writing first message

b3b296e

minor fixes

1e9ec04

fix script to mirror variable name change

de5f189

fix OutdatedRegionalSearchServiceInstanceIncidentParser

910b151

use shared table classes

5fa8140

increase nuget.services.status reference

f647d82

use shared incident api library

5328acf

increase to correct version of incident api

a884640

logging, comments, di

f8e3240

reduce logger scope api

10e159a

loic-sharma reviewed Jul 25, 2018

View reviewed changes

loic-sharma reviewed Jul 26, 2018

View reviewed changes

Scott Bommarito added 2 commits July 27, 2018 13:05

regex compliancy

97ae116

other pr fixes

40e34cd

loic-sharma reviewed Jul 27, 2018

View reviewed changes

Scott Bommarito added 2 commits July 27, 2018 16:22

simplify message logic

83d18a9

argument null exception

fd67157

cristinamanum approved these changes Jul 27, 2018

View reviewed changes

Scott Bommarito added 4 commits July 30, 2018 09:37

remove try get enum

4dd7933

Merge branch 'dev' into sb-icm

e6fbbdf

add back statusaggregator tests

287ea8b

this one's for u loic

33e8f41

loic-sharma reviewed Jul 30, 2018

View reviewed changes

loic-sharma approved these changes Jul 30, 2018

View reviewed changes

move incident title log to scope

33bce66

scottbommarito merged commit f18edac into dev Jul 30, 2018

scottbommarito deleted the sb-icm branch July 30, 2018 19:20

joelverhagen added a commit that referenced this pull request Sep 27, 2019

Merge branch 'dev' into master (#494)

e94ba81

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

Status aggregator job (#494)

2e7e721

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

Merge branch 'dev' into master (#494)

f8bc071

Status aggregator job #494

Status aggregator job #494

Conversation

scottbommarito commented Jul 25, 2018

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

loic-sharma Jul 27, 2018 • edited Loading

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

loic-sharma Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristinamanum 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

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

loic-sharma Jul 27, 2018 •

edited

Loading

loic-sharma Jul 27, 2018 •

edited

Loading