Add status classes and tests #170

scottbommarito · 2018-07-10T19:07:28Z

These classes will be shared between the Gallery, Status page, and Status Aggregator job to emit and consume status about the service.

I suggest starting at Status.cs for the most context.

shishirx34 · 2018-07-10T21:32:38Z

1300 lines 😩

scottbommarito · 2018-07-10T22:27:28Z

@shishirx34 - Apologies. It should be an easy read for its length, however. It's a lot of interfaces and relatively simple code.

loic-sharma · 2018-07-10T22:51:38Z

build.ps1

@@ -105,7 +106,8 @@ Invoke-BuildStep 'Creating artifacts' { `
            "src\NuGet.Services.ServiceBus\NuGet.Services.ServiceBus.csproj", `
            "src\NuGet.Services.Validation\NuGet.Services.Validation.csproj", `
            "src\NuGet.Services.Validation.Issues\NuGet.Services.Validation.Issues.csproj", `
-            "src\NuGet.Services.Sql\NuGet.Services.Sql.csproj"
+            "src\NuGet.Services.Sql\NuGet.Services.Sql.csproj", `
+            "src\NuGet.Services.Status\NuGet.Services.Status.csproj"


csproj [](start = 61, length = 6)

Shouldn't you update test.ps1 too?

Good catch!

loic-sharma · 2018-07-10T22:52:56Z

src/NuGet.Services.Status/Component.cs

+            IEnumerable<IComponent> subComponents)
+            : this(name, description)
+        {
+            SubComponents = subComponents?.Select(s => new SubComponent(s, this))


Select [](start = 43, length = 6)

You should ToList this. The IEnumerable could be some crazy LINQ query that people will have to enumerate every time they call SubComponents.

Agreed. Also made this fix to several other component classes

loic-sharma · 2018-07-10T22:53:23Z

src/NuGet.Services.Status/Component.cs

+        public string Description { get; }
+        public abstract ComponentStatus Status { get; set; }
+        public IEnumerable<IComponent> SubComponents { get; }
+        IEnumerable<IReadOnlyComponent> IReadOnlyComponent.SubComponents => SubComponents;


IEnumerable [](start = 8, length = 11)

Should this guy be public?

Fixed the inheritance so that this property no longer needs to exist.

loic-sharma · 2018-07-10T22:58:28Z

src/NuGet.Services.Status/ComponentUtility.cs

+
+                if (component == null)
+                {
+                    break;


break [](start = 20, length = 5)

If I run GetByPath('root', 'this', 'doesnt', 'exist') and only the root component exists, this will return the root IReadOnlyComponent. Would it be better if this returned null instead? If so, I would rename the method to be GetByPathOrNull.

good catch!

This was a bug. I added tests to make sure it doesn't break again.

loic-sharma · 2018-07-12T01:32:21Z

src/NuGet.Services.Status/Message.cs

+
+namespace NuGet.Services.Status
+{
+    public class Message : IMessage


Message [](start = 17, length = 7)

Does this and Event need interfaces? What value does it add?

loic-sharma · 2018-07-12T01:33:27Z

src/NuGet.Services.Status/ReadOnlyComponent.cs

+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using Newtonsoft.Json;
+using System;


System [](start = 6, length = 6)

Looks like that VS setting to put System on top got turned off :(

loic-sharma · 2018-07-12T01:33:56Z

src/NuGet.Services.Status/NuGet.Services.Status.csproj

+      <Version>9.0.1</Version>
+    </PackageReference>
+  </ItemGroup>
+  <Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />


Import [](start = 3, length = 6)

This project should be signed.

forgot about that, added now

loic-sharma · 2018-07-12T01:44:30Z

src/NuGet.Services.Status/ComponentUtility.cs

+        /// Gets the subcomponent of <paramref name="component"/> by iterating its subcomponents in the order of <paramref name="componentNames"/>.
+        /// If none exists, returns <c>null</c>.
+        /// </summary>
+        public static TComponent GetByNames<TComponent>(this TComponent component, params string[] componentNames)


component [](start = 72, length = 9)

Extension methods let the this parameter be null, thus, you should check if component is null

chenriksson · 2018-07-12T16:29:51Z

src/NuGet.Services.Status/Status.cs

+    /// <summary>
+    /// Describes the status of an entire service.
+    /// </summary>
+    public class Status


Maybe use a more specific name -- ServiceStatus? NuGetServiceStatus?

I feel like NuGetStatus is a little redundant given that it's in the namespace NuGet.Services.Status. Additionally, there's nothing NuGet specific to this class (or any of the classes in this project).

I think ServiceStatus makes sense.

chenriksson · 2018-07-12T16:30:38Z

src/NuGet.Services.Status/Status.cs

+        /// <summary>
+        /// The <see cref="IReadOnlyComponent"/> that describes the entire service. Its <see cref="IReadOnlyComponent.SubComponents"/> represent portions of the service.
+        /// </summary>
+        public IReadOnlyComponent RootComponent { get; }


What does RootComponent represent? Gallery?

RootComponent is the parent of Gallery, Search, our CDN, etc.
Basically, it's a component that encapsulates the entire service and reports a status for the entire service.

I think I will rename this to ServiceRootComponent.

chenriksson · 2018-07-12T18:34:07Z

src/NuGet.Services.Status/IReadOnlyComponent.cs

+        /// <summary>
+        /// A list of subcomponents that make up this part of the service.
+        /// </summary>
+        new IEnumerable<IReadOnlyComponent> SubComponents { get; }


Is new necessary? Why can't it reuse the IComponentRoot<T>.SubComponents?

I'll remove this.

chenriksson · 2018-07-12T18:36:00Z

src/NuGet.Services.Status/IComponent.cs

+        /// <summary>
+        /// A list of writable subcomponents that make up this part of the service.
+        /// </summary>
+        new IEnumerable<IComponent> SubComponents { get; }


Is new necessary? Why can't it reuse IReadOnlyComponent.SubComponentsorIComponentRoot.SubComponents`?

I'll remove this, IComponentRoot<IComponent>.SubComponents is sufficient?

I forgot that this needs to exist so that IComponent.SubComponents is not ambiguous between IEnumerable<IReadOnlyComponent> and IEnumerable<IComponent>.

chenriksson · 2018-07-12T18:39:56Z

src/NuGet.Services.Status/Component.cs

+    /// <summary>
+    /// Default implementation of <see cref="IComponent"/>.
+    /// </summary>
+    public abstract class Component : ReadOnlyComponent, IComponent


Do you have examples somewhere of what the the hierarchy of these components is? What LeafComponents, PrimarySecondaryComponents, TreeComponents, etc.

Could there be more specific name? NuGetServiceComponent?

To clarify:

A consumer of status (gallery, status page) is going to only work with IReadOnlyComponent and its implementation, ReadOnlyComponent. Consumers can't set or modify status, so the status of any component doesn't change and status doesn't need to be recalculated at any point.

IComponent and its implementations (which are subclasses of Component) are to be used by status producers (status job) and allow setting status. Because they allow setting status, the status of any component can change at any moment. A change in the status of a subcomponent can affect the status of a component that is its root. The subclasses of Component propagate changes in status differently from subcomponents to root components.

I'll provide some examples for each Component subclass I have here:

TreeComponent:
V3 restore is a component with two subcomponents, one for each of its regions--Global and China.
If either Global or China is down, V3 restore is degraded because both Global and China are used by customers to restore using V3.

PrimarySecondaryComponent:
The gallery is a component with two subcomponents, one for each of its regions--USNC and USSC.
If USNC is up, we don't care about the status of USSC because it's only used if USNC is down.d

LeafComponent:
The USNC region of Gallery is a LeafComponent because it can't be broken down into any subcomponents.
(Although TreeComponent and PrimarySecondaryComponent both support not having subcomponents and behave as LeafComponent does, I created this subclass so that status producers don't have to choose between the two arbitrarily.)

I suggest adding comments w/ your examples to the code.

chenriksson · 2018-07-12T18:43:36Z

src/NuGet.Services.Status/Component.cs

+    public abstract class Component : ReadOnlyComponent, IComponent
+    {
+        public new abstract ComponentStatus Status { get; set; }
+        public new IEnumerable<IComponent> SubComponents { get; }


Is there another way to achieve this without the new to replace base properties? It makes the code more complex.

Unfortunately no, but I think from a consumer side this makes these classes a lot easier to use because I'm replacing the base properties in an intuitive way. I tried doing this without replacing any properties and it made the code substantially uglier.

I replace Status to add a setter to a property that is read-only on the base interface.

I replace SubComponents to provide the additional restriction that all members of the property on the base interface are of a specific subclass (IComponent instead of IReadOnlyComponent).

chenriksson · 2018-07-12T18:57:38Z

src/NuGet.Services.Status/ReadOnlyComponentWrapper.cs

+        public IEnumerable<IReadOnlyComponent> SubComponents { get; }
+        public string Path => _parent.Path + Constants.ComponentPathDivider + Name;
+
+        public ReadOnlyComponentWrapper(IReadOnlyComponent component, IReadOnlyComponent parent)


Is there an alternative to wrapper classes for assigning parent/path? Makes the code more complex, and encapsulates the component types that are wrapped.

A component only has a path when accessed as a subcomponent of another component. If we don't wrap or clone the subcomponents, a user could get some very strange results if they reuse the same component in multiple component trees.

In other words:

var a = new Component(); var b = new Component(subcomponents: new[] { a }); var c = new Component(subcomponents: new[] { a }); Console.WriteLine(b.SubComponents.First().Path); // should return "b/a" Console.WriteLine(c.SubComponents.First().Path); // should return "c/a"

If the constructor of Component set the path of a and did not wrap or clone a, the path set by c's constructor will override the path set by b's constructor, which would create unintended results when accessing a as a subcomponent of b.

I chose wrapping instead of cloning because I think it would be confusing to have

var a = new Component(); var b = new Component(subcomponents: new[] { a }); a.Status = ComponentStatus.Degraded; Console.WriteLine(b.SubComponents.First().Status); // outputs ComponentStatus.Up if cloned // and ComponentStatus.Degraded if wrapped

Note that this class is also internal and is meant to be invisible to the consumer.

I assumed a component would only have a single parent, and would either be child or root node. Guess that's not the case?

Maybe Node would be better suffix than Wrapper since it implies a relational object?

I don't expect to be creating any components with multiple parents, but I felt this model was easier to use than a model asserting a single parent.

I think I'll add some of the discussion above to a comment in the code to make it clearer. Ultimately, given this is an internal class, I don't think naming is super important as long as the purpose of the class is communicated well.

chenriksson · 2018-07-12T22:46:58Z

src/NuGet.Services.Status/ReadOnlyComponentWrapper.cs

+        public IEnumerable<IReadOnlyComponent> SubComponents { get; }
+        public string Path => _parent.Path + Constants.ComponentPathDivider + Name;
+
+        public ReadOnlyComponentWrapper(IReadOnlyComponent component, IReadOnlyComponent parent)


I assumed a component would only have a single parent, and would either be child or root node. Guess that's not the case?

Maybe Node would be better suffix than Wrapper since it implies a relational object?

Scott Bommarito added 3 commits July 9, 2018 18:05

add status classes and tests

15fd822

add comments

4e12b54

add copyright

72f5ffd

scottbommarito requested review from joelverhagen, agr, loic-sharma, xavierdecoster, shishirx34, ryuyu, anangaur, chenriksson, cristinamanum and skofman1 July 10, 2018 19:07

Scott Bommarito added 2 commits July 10, 2018 12:58

Merge branch 'master' into sb-status

1ffe907

add getbypath

9b15827

Scott Bommarito added 2 commits July 10, 2018 15:01

add GetPath

c20ff3d

IComponent GetByPath

88a025f

loic-sharma reviewed Jul 10, 2018

View reviewed changes

Scott Bommarito added 3 commits July 11, 2018 16:54

improve inheritance of icomponent

b5be64a

added null tests for getbypath and getbynames

4c6bf3e

Merge branch 'master' into sb-status

7fbd34e

loic-sharma reviewed Jul 12, 2018

View reviewed changes

loic-sharma approved these changes Jul 12, 2018

View reviewed changes

pr fixes

bc7b870

chenriksson reviewed Jul 12, 2018

View reviewed changes

rename component root to root component

c17481d

chenriksson approved these changes Jul 12, 2018

View reviewed changes

Scott Bommarito added 2 commits July 12, 2018 16:21

style fixes

7eaf4e3

fix deserialization bug

46d9624

scottbommarito merged commit 4aeb1a3 into master Jul 13, 2018

scottbommarito deleted the sb-status branch July 13, 2018 00:00

scottbommarito mentioned this pull request Jul 28, 2018

Status aggregator job NuGet/NuGet.Jobs#494

Merged

Add status classes and tests #170

Add status classes and tests #170

Conversation

scottbommarito commented Jul 10, 2018

shishirx34 commented Jul 10, 2018

scottbommarito commented Jul 10, 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

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