Tag enhancement #12

MattMofDoom · 2022-02-10T00:25:25Z

This pull will resolve and close #11

This pull can supersede and close #10 - it includes the added logging and OpsGenie properties that this pull request added

The System.Text.Json dependency was updated for this request.

Update from dev

…into dev

…clean up classes and interfaces

Fix product name

…p-opsgenie into BetterLogging

MattMofDoom · 2022-04-17T23:13:29Z

@nblumhardt I've closed #9 and #10 as this PR supersedes them.

This PR will resolve #11 while adding better logging, adding the Seq event properties to the Opsgenie alert, and implementing a suppression setting.

nblumhardt

Hi @MattMofDoom! Ran through quickly; a few interlocking changes in this PR, I think I covered them all but might be easier and faster to review as small/individual/focused PRs next time around :-) .. Let me know if I've misread anything. Cheers!

nblumhardt · 2022-04-19T06:17:25Z

src/Seq.App.Opsgenie/Classes/OpsGenieData.cs

+
+// ReSharper disable ClassNeverInstantiated.Global
+
+namespace Seq.App.Opsgenie.Classes


Organizing into Classes and Interfaces might not give as much insight into the structure of the codebase as some alternatives - could this one go in Seq.App.Opsgenie.Api, perhaps, since the structure of these types (presumably) matches the requirements of the Opsgenie API?

Yeah should be fine. I'll take a look.

Reorganised into Api and Client

nblumhardt · 2022-04-19T06:18:25Z

src/Seq.App.Opsgenie/Interfaces/IOpsgenieApiClient.cs

+
+namespace Seq.App.Opsgenie.Interfaces
+{
+    interface IOpsgenieApiClient


This one might go in Seq.App.Opsgenie.Client?

Reorganised into Api and Client

nblumhardt · 2022-04-19T06:18:47Z

src/Seq.App.Opsgenie/OpsgenieApiClient.cs

@@ -6,15 +6,15 @@
 using System.Text.Json;
 using System.Text.Json.Serialization;
 using System.Threading.Tasks;
-using Seq.Apps;
+using Seq.App.Opsgenie.Classes;
+using Seq.App.Opsgenie.Interfaces;

 namespace Seq.App.Opsgenie
 {
    class OpsgenieApiClient : IOpsgenieApiClient, IDisposable


This type could also move to Seq.App.Opsgenie.Client

Reorganised into Api and Client

nblumhardt · 2022-04-19T06:21:05Z

src/Seq.App.Opsgenie/OpsgenieApiClient.cs

-            if (!response.IsSuccessStatusCode)
+                result.Response = opsGenieResponse;
+            }
+            catch (Exception ex)


Any reason not to just let the exception bubble up, here, and be handled by the hosting environment? In general, Seq apps shouldn't customize error handling, since the app host does this consistently for all hosted apps and in future we might make improvements up at that layer.

I think the perspective was to allow a consistent approach to error handling based on a signal targeted to the specific appid or appinstanceid (or instance name) that included the context/properties around what the alert was. So for example if we fail to connect to OpsGenie to send the alert, we can send an email or Teams or JIRA alert that preserves the context of what we were alerting, by having an error event that preserves this.

I'm still inclined to have this caught for the reasons above. It's fairly serious if an alert doesn't go, and if we can catch what the alert was, then we have opportunity to send an alert via other alerting apps. Overall this speaks to Seq reliability - being able to do fallbacks etc. Unfortunately the hosting environment doesn't preserve the event data ... I suppose it could still throw after this?

nblumhardt · 2022-04-19T06:24:09Z

src/Seq.App.Opsgenie/OpsgenieApp.cs

+            HelpText =
+                "If an alert has been raised, further alerts will be suppressed for this time. Default 60, Minimum 0.",
+            InputType = SettingInputType.Integer)]
+        public int SuppressionTime { get; set; } = 60;


Suppression time is provided by Seq (for regular alerts it's called suppression time; for streamed events its "debounce window"), it'd be preferable not to duplicate it here.

Removed suppression time

nblumhardt · 2022-04-19T06:30:35Z

src/Seq.App.Opsgenie/OpsgenieApp.cs


            var alert = new OpsgenieAlert(
                message,
                evt.Id,
                description,
                priority.ToString(),
                responders,
+                new Dictionary<string, string>


This info should generally come from the event directly; e.g. based on the event type, events carry the URL that the user can navigate to in order to see relevant info. There are three branches in:

https://github.com/datalust/seq-app-htmlemail/blob/dev/src/Seq.App.EmailPlus/Resources/DefaultBodyTemplate.html#L105

based on the event type.

Alerts carry different data from regular events; the template shows some of this.

The Slack app has some helper types to achieve something along the lines of what you'll need in order to add this feature here:

https://github.com/bytenik/Seq.App.Slack/tree/master/src/Seq.App.Slack/Messages

Could be nice to bring into this codebase (but rather than as a "formatter", perhaps as "payload builders")? Seems deep enough to need a separate PR :)

Yeah, can't recall if this was before I was across the alerting changes or not. It's useful to have the data show up in Opsgenie so I'll take a look at the slack helpers to start with

Not 100% sure how to proceed with this. It looks like only NameSpacedAlertTitle and event URL would be affected? I think the latter would also have to be amended in the HandlebarsTemplate.

Overall the goal would be to include the raw seq properties in the OpsGenieAlert, which makes this a more informative alert when examining in OpsGenie. Tested when I had access to Opsgenie but a bit harder to visualise now.

nblumhardt · 2022-04-19T06:33:08Z

src/Seq.App.Opsgenie/OpsgenieApp.cs


            var message = _generateMessage.Render(evt);
            var description = _generateDescription.Render(evt);
            var priority = ComputePriority(evt);
-            var tags = ComputeTags(evt);
+            var tags = ComputeTags(evt, _includeTags, _includeTagProperty, _tags);


Any chance of an example (screenshot of config?) showing how tag configuration would work in practice with this change?

Yeah I should be able to dig it up. The difference was that it was not handling a comma delimited string passed from a Seq alert, because it was always expecting an array. This allows either to work as expected.

@nblumhardt An example is shown below.

If I configure the Notification property tags on an alert, the current (v1.0) version of Seq.App.OpsGenie will ignore the comma-delimited tags sent from the alert, and only send the configured tags on the app instance.

With the enhancement, my comma-delimited set of tags will be used, although I'm not sure that the configured app instance tags are being included (it should be combining the OpsGenie alert tags with the event tags), and the whole set of tags isn't being logged - I'll have to check that.

Alert config:

OpsGenie instance tags config:

@nblumhardt This is a bit curious. The unit tests for tag calculation work correctly - if a static tag is configured in the OpsGenie instance and an event passes a comma-delimited set of tags, it correctly combines the static tag with the event tags.

However in the DEV instance, thanks to the Handlebars template I'm using, I can see in the Opsgenie alert that Tags was passed from the alert and shows within the alert description, but doesn't actually get passed to OpsGenie as a Tags property. The static tag ("Test") does get passed and this is seen in the Seq logging as well.

Handlebars template is
<p>{{AppName}} failed with the error {{Description}}<br /><br /></p><table>{{#each $Properties}}<tr><td><b>{{@key}}:</b></td><td>{{this}}</td></tr>{{/each}}</table><p>Alert created by Seq</p>

and the outcome in OpsGenie is below. You can see the tag ("Test") just below the title, but you can see in the description that the Handlebars template has shown the Tags property from the event passed by Alerting.

Suggests a problem in the ComputeTags method, but I don't see it - can't reproduce with a test case that emulates what gets passed. This is complicated by the fact that I'm leaving my current job so won't have access to their OpsGenie for testing in future ☹️

@nblumhardt I've amended the test case to evaluate a calculated OpsgenieAlert, for passing either an array or a comma separated value, and I do think it's working as intended. I don't know what I was seeing on the OpsGenie side, but at the end of the day, if the calculated OpsGenieAlert has the tags, they will be passed to Opsgenie.

src/Seq.App.Opsgenie/Seq.App.Opsgenie.csproj

MattMofDoom and others added 16 commits June 17, 2021 09:55

Merge pull request #2 from datalust/dev

529302d

Update from dev

Merge branch 'datalust:dev' into dev

a4cf7a4

Update description to LongText input type

b1b8cbf

Merge branch 'dev' of https://github.com/MattMofDoom/seq-app-opsgenie …

64a0eb7

…into dev

Update Handlebars.NET

40b40d8

Merge branch 'datalust:dev' into dev

e5187e9

Add optional suppression timeframe

b3d52a8

Merge branch 'dev' of https://github.com/MattMofDoom/seq-app-opsgenie …

1bcdeb3

…into dev

Improve logging of OpsGenie results, add Seq properties to OpsGenie, …

7120c43

…clean up classes and interfaces

Fix product name

4ae80e2

Merge branch 'BetterLogging' into dev

55dcd2b

Merge pull request #3 from MattMofDoom/dev

ddff0cb

Fix product name

Update dependencies

723ca69

Merge branch 'BetterLogging' of https://github.com/MattMofDoom/seq-ap…

0659eaa

…p-opsgenie into BetterLogging

Allow tags to be passed as a comma-delimited string

2b79763

Update Microsoft.NET.Test.SDK

f5a7b88

This was referenced Apr 17, 2022

Add suppression setting #9

Closed

Improved logging, add Seq properties to OpsGenie #10

Closed

nblumhardt reviewed Apr 19, 2022

View reviewed changes

MattMofDoom added 5 commits May 19, 2022 09:42

Re-work test cases to evaluate OpsGenieAlert

71ea744

Remove Version tag

89cb77e

Reorganise to Api and Client, remove suppression time

5c81208

Update dependencies

308c7c0

Update dependencies

493c101

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tag enhancement #12

Tag enhancement #12

MattMofDoom commented Feb 10, 2022

MattMofDoom commented Apr 17, 2022

nblumhardt left a comment

nblumhardt Apr 19, 2022

MattMofDoom Apr 19, 2022

MattMofDoom May 19, 2022

nblumhardt Apr 19, 2022

MattMofDoom May 19, 2022

nblumhardt Apr 19, 2022

MattMofDoom May 19, 2022

nblumhardt Apr 19, 2022

MattMofDoom Apr 19, 2022

MattMofDoom May 19, 2022

nblumhardt Apr 19, 2022

MattMofDoom May 19, 2022

nblumhardt Apr 19, 2022

MattMofDoom Apr 19, 2022

MattMofDoom May 18, 2022

nblumhardt Apr 19, 2022

MattMofDoom Apr 19, 2022

MattMofDoom Apr 20, 2022

MattMofDoom Apr 20, 2022

MattMofDoom May 18, 2022


		// ReSharper disable ClassNeverInstantiated.Global

		namespace Seq.App.Opsgenie.Classes

Tag enhancement #12

Are you sure you want to change the base?

Tag enhancement #12

Conversation

MattMofDoom commented Feb 10, 2022

MattMofDoom commented Apr 17, 2022

nblumhardt 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