The XML comments to the public members of Nimator were added by one person and reviewed by that same person, so they are bound to have these kinds of problems:

• Spelling and grammar mistakes;
• Incorrect usage of special xml-doc elements;
• Unclear phrasings, overly verbose stuff, too concise bits;
• Etc.

It would be good if some interested party goes through all of them, suggesting improvements in fresh commits. If anything is unclear, this issue could probably (up to a point) serve for docs-related-questions.

An INotifier typically has a threshold (it probably does if it uses the NotifierSettings base class). The built in notifiers use this to decide whether to actually send out the grand result of a cycle.

However, the actual message is typically created using INimatorResult.RenderPlainText, which in turn calls ToString on the inner LayerResults (see also #2 on that topic), which has a hard-coded assumption about the level for which the ICheckResult details are rendered:

public override string ToString()
{
    var checkErrors = CheckResults
        .Where(r => r.Level >= NotificationLevel.Error)
        .Select(r => r.ToString());
// etc.

This effectively means a INotifier that has a threshold of Warning will still not send out the contents of warning check results.

Probably the RenderPlainText needs to be supplied a threshold, which can be passed down to the other render methods, to display the appropriate information? Again, see also #2.

The "attachment" (in the Slack-API-sense) messages no longer show up when SlackNotifier pushes an update.

That is, previously there would be two "attached" messages in addition to the fully rendered Nimator result:

• Level message with high-level description (e.g. "Failure in [LayerXyz], checks [Check1, Check2, Check3]") and color corresponding to that level;
• Blue attachment with message about the next message being debounced until a certain moment in time, if configured;

Possibly one of the refactors changed the way the classes are pushed to the REST api? Perhaps some explicit serialization info is needed, should this bug be caused by property renames...

It would make a lot of sense to add an implementation of INotifier (and corresponding NotifierSettings subclass) that will send notifications to a selected list of e-mail addresses, via a specified mail server.

Though it's unclear at this point what the exact details would be, at the least it will very likely need to include:

• A debounce feature, so you don't accidentally spam people;
• To/Cc/Bcc support (incl multiple addressees);
• Format for the subject line with both "set" text and (parts of) the failing checks/layers;
• Body with all the details of the failed message;
• Helpful logging if e-mailing goes wrong (e.g. mailbox unavailable errors should end up in the application error log);

Suppose you have these settings:

{
  "Notifiers": [ ],
  "Layers": [{ "Name": "Demo layer 1", "Checks": [] }]
}

Then Nimator will just start running but never output anything.

It's an easy mistake to make, having no notifiers. If this would happen, a user would probably check the log4net output to see why this happens, but find nothing.

So it seems quite useful to have log4net output a warning if this happens.

If you have this first version for settings...

{
  "Layers": [{ "Name": "Demo layer 1", "Checks": [] }]
}

...then you will effectively get a ConsoleNotifier because the default constructors instantiate one.

However, this second version for settings...

{
  "Notifiers": [ ],
  "Layers": [{ "Name": "Demo layer 1", "Checks": [] }]
}

...will not notify anything (see also #4).

The issue is that the difference in behavior can be quite confusing. Once #4 is fixed we could have the first version also default to zero notifiers. What's possible both before and after #4 is that the second version also gets a default notifier.

The Nimator.TickSafe() implementation is typically called on a timer (just like the example console app does). However, the checks that are configured possibly take longer than the timer interval, causing monitoring cycles to be fired more quickly than they can be finished. This is obviously a problem.

Although it can be argued that this problem should be solved in the calling application that has the timer, it also makes sense to have a more robust framework in this regard. Some possibilities would include:

• Throwing an exception from TickSafe whenever a previous call is still in progress;
• Syncrhonize all calls to TickSafe by blocking subsequent calls until the previous ones finish;
• Aborting a previous TickSafe with an exception and/or failed result whenever a new call is started when an old one is still in progress;
• Log a Warning if a new call is started when old calls are still in progress;
• Etc.

At any rate, this is something that should possibly be handled by the framework.

2016-10-18 13:36:53,648 [98] ERROR OmniaMonitoring - App tick caused a NotificationException.
Nimator.NotificationException: One or more notifiers could not notify. ---> System.OutOfMemoryException: Exception of type 'System.OutOfMemoryException' was thrown.
   at System.Net.ScatterGatherBuffers.AllocateMemoryChunk(Int32 newSize)
   at System.Net.ScatterGatherBuffers..ctor(Int64 totalSize)
   at System.Net.WebClient.DownloadBitsState.SetResponse(WebResponse response)
   at System.Net.WebClient.DownloadBits(WebRequest request, Stream writeStream, CompletionDelegate completionDelegate, AsyncOperation asyncOp)
   at System.Net.WebClient.UploadDataInternal(Uri address, String method, Byte[] data, WebRequest& request)
   at System.Net.WebClient.UploadString(Uri address, String method, String data)
   at System.Net.WebClient.UploadString(String address, String data)
   at Nimator.Util.SimpleRestUtils.PostToRestApiInternal(String url, Object message) in D:\buildAgent\work\a39b915f9d70f079\src\Nimator\Util\SimpleRestUtils.cs:line 24
   at Nimator.Notifiers.OpsGenieNotifier.SendHeartbeat() in D:\buildAgent\work\a39b915f9d70f079\src\Nimator\Notifiers\OpsGenieNotifier.cs:line 37
   at Nimator.Notifiers.OpsGenieNotifier.Notify(INimatorResult result) in D:\buildAgent\work\a39b915f9d70f079\src\Nimator\Notifiers\OpsGenieNotifier.cs:line 27
   at Nimator.Nimator.<>c__DisplayClass4_0.<Tick>b__0(INotifier notifier) in D:\buildAgent\work\a39b915f9d70f079\src\Nimator\Nimator.cs:line 82
   --- End of inner exception stack trace ---
   at Nimator.Nimator.Tick() in D:\buildAgent\work\a39b915f9d70f079\src\Nimator\Nimator.cs:line 92
   at Nimator.Nimator.TickSafe(ILog logger) in D:\buildAgent\work\a39b915f9d70f079\src\Nimator\Nimator.cs:line 51
---> (Inner Exception #0) System.OutOfMemoryException: Exception of type 'System.OutOfMemoryException' was thrown.
   at System.Net.ScatterGatherBuffers.AllocateMemoryChunk(Int32 newSize)
   at System.Net.ScatterGatherBuffers..ctor(Int64 totalSize)
   at System.Net.WebClient.DownloadBitsState.SetResponse(WebResponse response)
   at System.Net.WebClient.DownloadBits(WebRequest request, Stream writeStream, CompletionDelegate completionDelegate, AsyncOperation asyncOp)
   at System.Net.WebClient.UploadDataInternal(Uri address, String method, Byte[] data, WebRequest& request)
   at System.Net.WebClient.UploadString(Uri address, String method, String data)
   at System.Net.WebClient.UploadString(String address, String data)
   at Nimator.Util.SimpleRestUtils.PostToRestApiInternal(String url, Object message) in D:\buildAgent\work\a39b915f9d70f079\src\Nimator\Util\SimpleRestUtils.cs:line 24
   at Nimator.Notifiers.OpsGenieNotifier.SendHeartbeat() in D:\buildAgent\work\a39b915f9d70f079\src\Nimator\Notifiers\OpsGenieNotifier.cs:line 37
   at Nimator.Notifiers.OpsGenieNotifier.Notify(INimatorResult result) in D:\buildAgent\work\a39b915f9d70f079\src\Nimator\Notifiers\OpsGenieNotifier.cs:line 27
   at Nimator.Nimator.<>c__DisplayClass4_0.<Tick>b__0(INotifier notifier) in D:\buildAgent\work\a39b915f9d70f079\src\Nimator\Nimator.cs:line 82<---

Nimator uses simple WebClient's in the SimpleRestUtils.cs file, which has two problems:

• the default timeout is kind of big (should e.g. OpsGenie or Slack not be available), 1 minute I believe, this can easily be 5 or 10 seconds, tops
• you might want to turn AllowWriteStreamBuffering to false, see: https://bytes.com/topic/c-sharp/answers/255250-system-net-scattergatherbuffers-memorychuck-allocates-large-byte

public class CustomWebClient : WebClient
{
    private readonly TimeSpan timeout;

    public CustomWebClient(TimeSpan timeout)
    {
        this.timeout = timeout;
    }

    protected override WebRequest GetWebRequest(Uri address)
    {
        HttpWebRequest request = (HttpWebRequest)base.GetWebRequest(address);

        request.Timeout = (int)this.timeout.TotalMilliseconds;
        request.AllowWriteStreamBuffering = false;

        return request;
    }
}

Both ICheckResult and (obiviously) CheckResult have setters for these properties:

public NotificationLevel Level { get; set; }

and

public string CheckName { get; set; }

We found while dogfooding that it often makes sense to have an implementation of ICheckResult that does not have such a setter for e.g. the Level, because it's a read-only / derived property. There was no sensible or useful implementation for the setter.

In addition, currently these setters aren't used anywhere in Nimator itself either.

So, we might as well get rid of these setters.

The settings for checks tend to attract sensitive data like usernames/passwords to connect to e.g. database systems. While of course it would be great to have "least priviliged" account info for monitoring, they're accounts nonetheless.

Now true, Nimator just asks for json with settings, and doesn't tell the application anything on how to persist it safely. However, the ExampleApplication and getting started both suggest using plain text json in an embedded resource or config file, which could lead to unsafe situations.

Any advice or possibly even coding mechanisms to improve this situation, without attracting heavy dependencies (as a monitoring application should be as standalone as possible) from Nimator would be nice.

It's nice that there's an interface for each level of result (top level, layers, checks), but the interface doesn't say anything about how the results can be rendered.

The default implementation of the top two levels (NimatorResult and LayerResult) now both do some string magic to render the end result (i.e. when NimatorResult.RenderPlainText() is called), but internally they use ToString() on the levels below them. Effecitively:

• ILayerResult implementors need to override ToString()
• ICheckResult implementors need to override ToString()

or they will not be rendered in the RennderPlainText() output properly.

Being rendered seems to be a main task of both levels of results, so the interfaces for them should probably be explicit too. Perhaps both of them need to have their own RenderPlainText() definitions, so that it is clear that implementations need that to render the result properly?

There is an ExampleConsoleApp wrapper, but most likely Nimator will be used as a Windows Service in production scenarios. It could help to have an example implementation for this.

Repro:

1. Clone the repository to a fresh location.
2. Open the sln in VS2015.
3. Build.

Result:

Error: Unable to locate 'C:\Users\jeroen\Documents\github\nimator\src.nuget\NuGet.exe'

Expected: Build successful.

Consider a settings file with "Layers": [] or no Layers property at all (which defaults to an empty list as well). In this case a Nimator cycle ends with this exception:

App tick caused a NotificationException.
Nimator.NotificationException: One or more notifiers could not notify.
---> System.InvalidOperationException: Sequence contains no elements
   at System.Linq.Enumerable.Max(IEnumerable`1 source)
   at System.Linq.Enumerable.Max[TSource](IEnumerable`1 source, Func`2 selector)
   at Nimator.NimatorResult.get_Level() in D:\github\Nimator\src\Nimator\NimatorResult.cs:line 24
   at Nimator.Notifiers.ConsoleNotifier.Notify(INimatorResult result) in D:\github\Nimator\src\Nimator\Notifiers\ConsoleNotifier.cs:line 18
   at Nimator.Nimator.<>c__DisplayClass4_0.<Tick>b__0(INotifier notifier) in D:\github\Nimator\src\Nimator\Nimator.cs:line 75
   --- End of inner exception stack trace ---
   at Nimator.Nimator.Tick() in D:\github\Nimator\src\Nimator\Nimator.cs:line 85
   at Nimator.Nimator.TickSafe(ILog logger) in D:\github\Nimator\src\Nimator\Nimator.cs:line 44
---> (Inner Exception #0) System.InvalidOperationException: Sequence contains no elements
   at System.Linq.Enumerable.Max(IEnumerable`1 source)
   at System.Linq.Enumerable.Max[TSource](IEnumerable`1 source, Func`2 selector)
   at Nimator.NimatorResult.get_Level() in D:\github\Nimator\src\Nimator\NimatorResult.cs:line 24
   at Nimator.Notifiers.ConsoleNotifier.Notify(INimatorResult result) in D:\github\Nimator\src\Nimator\Notifiers\ConsoleNotifier.cs:line 18
   at Nimator.Nimator.<>c__DisplayClass4_0.<Tick>b__0(INotifier notifier) in D:\github\Nimator\src\Nimator\Nimator.cs:line 75<---

There is no need for this exception to occur.

The expected result is probably a Warning notification instead, analogous to the situation where an empty Layer generates a Warning-level result ("Warning: no checks in this layer.").

The Nimator.ExampleConsoleApp has this example code:

var json = reader.ReadToEnd();
return Nimator.FromSettings(logger, json);

However, if you create a new fresh console application and reference Nimator, and copy-paste said code to your code file, you'll not be able to compile. Instead, you need to do this:

var json = reader.ReadToEnd();
return Nimator.Nimator.FromSettings(logger, json); // <-- Subtly different! "Nimator.Nimator..."

This is caused by the fact that the default namespace for the example app is part of a sub-namespace (Nimator.ExampleConsoleApp) of the Nimator namespace, whereas the fresh console app isn't.

Bottom line: it's confusing to have both a class and a namespace called "Nimator". Please consider changing this setup.

Suppose these settings for a notifier:

{
  "$type": "Nimator.Settings.OpsGenieSettings, Nimator",
  "Threshold": "Error",
  "ApiKey": "correct-api-key-here",
  "TeamName": "ops_team",
  "HeartbeatName":  "nimator"  
}

If you have a default trial account with OpsGenie, this will generate this kind of error:

Nimator.NotificationException: One or more notifiers could not notify. ---> Nimator.NotificationCommunicationException: Notification failed with '400 BadRequest' for Url https://api.opsgenie.com/v1/json/heartbeat/send ---> System.Net.WebException: The remote server returned an error: (400) Bad Request.

It does so because by default there is no heartbeat avaible. (If you create a heartbeat of the correct name things will work, obviously.)

Removing the HeartbeatName from the settings entirely, or setting it to an empty string will cause an ArgumentException in the OpsGenieNotifier constructor. However, it would be quite reasonable to have no heartbeat configured in case you don't want to use them.

Long story short: please make HeartbeatName optional, and don't try to push heartbeat notifications if it IsNullOrWhitespace.