datalust / seq-app-opsgenie

Create Opsgenie alerts in response to events or notifications in Seq
Apache License 2.0
3 stars 2 forks source link

Add DefaultResponders #7

Closed MattMofDoom closed 3 years ago

MattMofDoom commented 3 years ago

Hi @nblumhardt,

Followon to #6, adding the default responders property as mentioned in my last comment. Enabling mapping of a responder property is now conditional on ResponderProperty, Responder, and Default Responder being configured.

Further to this, I noted and fixed a suspicious comparison that I'd created in #6 for the ComputerResponders. Stray brackets...

As we have add some complexity to the config with the new properties and especially property mappings, I have added debug logging so that the outcome of these configurations can be derived from the logs. This should help with pinpointing misconfigurations or unexpected behaviours.

I also ran OpsGenieApp through a code cleanup and took a couple of code usage opportunities (eg. using switch statements, etc). TryGetPropertyValueCI was renamed for the sake of a constraints violation.

Resulting changes look like more than they are as a result of methods being reordered - apologies.

Cheers,

Matt

nblumhardt commented 3 years ago

Thanks Matt! I'll dig in as soon as I have a sec πŸ‘

Regarding CI, because it's an acronym in this context (Case Insensitive), the casing should be CI, but by default, Rider/ReSharper doesn't always know what is an acronym vs a two-letter word. (The example given in the guidelines is IOStream.)

You should be able to Alt+Enter on it (when the squiggly is showing), and find an option like "add CI to acronyms list" - I thought I'd done that, here, but it seems like a few code style things haven't been saved alongside the project successfully (none of the code uses an explicit private modifier in this project, either).

MattMofDoom commented 3 years ago

@nblumhardt No worries. Probably just a case of over-enthusiasm in helping out.

nblumhardt commented 3 years ago

Not at all! Need as much enthusiasm as we can get πŸ˜… ... we usually manage to save all these settings alongside the project, I'll try to figure out what's missing. Have a great weekend!

nblumhardt commented 3 years ago

Merged! Thanks Matt πŸ‘

MattMofDoom commented 3 years ago

Thanks @nblumhardt πŸ˜€

MattMofDoom commented 3 years ago

@nblumhardt Testing results for you. I held off on testing #6 to be able to do a full test with the #7 enhancements.

I had already released the update to Seq.App.EventTimeout that allowed it to log priority and responder properties with its timeout events, so I just needed an instance that would time out without a match.

This is successful! πŸ˜€The priority and responder match those passed by Event Timeout when sent to OpsGenie. This includes comma-delimited list of responders.

I further tested the default priority and default responder properties, by sending priorities and responders that didn't match the Seq.App.OpsGenie configuration. This was also successful.

One slightly clunky thing is that I realised that when I have the ability to specify a priority in Event Timeout, it means that if I configure (for example) P3, I still need a matching priority mapping to exist in Seq.App.OpsGenie, eg. P1=P1,P2=P2,P3=P3,P4=P4,P5=P5. That's not totally without merit - we might want to remap priorities, and in fact I do. P5 is not used for us, so I can remap a P5 to a P4, so that if this ever happens, we won't suddenly get a P5 alert.

I think it's a reasonable tradeoff for the ability to make priority mappings totally configurable, so my inclination is to let that requirement stand. It also means that multiple apps could pass a Priority property with different values, which means we can configure a list that maps any possible value to an OpsGenie priority.

The other finding relates to the debug logging that I added for Responders and Priorities. It's "okay", but it includes the $type in the message, eg

Default Responders: [{"Username":null,"Name":"Windows Escalation","Type":"Escalation","$type":"Responder"}]

Obviously the Default Responders property itself is rendered neatly when examined. This is obviously a side effect of including the destructured object in the message... inclined to let it pass since it's strictly a debug message.

The only further improvement I could think of would be to pass the instance title as part of debug message and properties, but it's not in the current pattern for Seq.App.OpsGenie logging, so I think this is okay as it stands.

MattMofDoom commented 3 years ago

@nblumhardt I've taken a pass at covering off how this all hangs together ... obviously with an eye to how it works with Event Timeout, but thought you may be interested. https://mattmofdoom.com/passing-priority-responder-and-tags-from-seq-to-opsgenie/

Have a great weekend, and thanks for all your encouragement and assistance! πŸ˜€

nblumhardt commented 3 years ago

Wow, very nice! We should link your post from the README.. BRB! :-)

MattMofDoom commented 3 years ago

Thanks Nicholas! 😊