search

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

Style nits; break out priority/tag/responder mapping so we can write some tests #4

Closed nblumhardt closed 3 years ago

nblumhardt commented 3 years ago

Round 1 was Rider-driven restyling, which led me to round 2 -

Without being able to test on a live OpsGenie account, I have trouble making sure my changes don't break anything. So, I had a shot at breaking apart the OpsGenieApp.OnAsync() function to support some simple unit testing.

The tests in OpsGenieAppTests now cover mapped priority calculation, but I'm out of time and had to stop there.

Ideally, the next time we dig in, we can add some similar tests for non-mapped priorities, for tags, and for responders (I'm not 100% certain I extracted the logic for the latter without breaking anything - would be great to have another set of eyes on it 😅 ).

Unfortunately, the diff is hard to follow due to the code movement; hopefully breaking some ground on unit tests will justify the churn this time around.

// CC @MattMofDoom

MattMofDoom commented 3 years ago

Thanks @nblumhardt ... I can try firing some tests at OpsGenie with the NuGet build over the next day for the sake of sanity. I think I'll have to bite the bullet and invest in a dotUltimate license for myself, I always love the results.

nblumhardt commented 3 years ago

1.0.0-dev-00021 is on NuGet with these changes 😎

MattMofDoom commented 3 years ago

@nblumhardt With 1.0.0-dev-00021, I was able to define an instance that used the default Priority property of @Level (ie. I left it blank), and set Alert Priority or Property Mapping to "Error=P1,Warning=P2,Information=P3,Debug=P4,Verbose=P5". I set the default priority as P3.

I sent an event with @Level=Debug to the instance, and it correctly raised as P4.

I also included the new handlebars template $EventUri in the alert description, and it did correctly reflect a link to the event as expected 😊

I then came up with a rough and ready way to use Event Timeout to pass through a property that I could match for the Responder property mapping. However I did hit a snag.

To avoid sending unnecessary alerts to anyone else, I was matching against a ResponderType.User. I could see from the debug logs that we did correctly send a responders field to Opsgenie, but the Opsgenie logging indicated no responder. It was directed to me anyway, because I had a "test" tag setup for the purpose, but I dug a bit deeper.

The OpsGenie alert API uses the "name" field for Team, Type, and Schedule. For User, it uses the "username" field. https://docs.opsgenie.com/docs/alert-api

I have an idea on how to handle that so I'll send over another pull request with the implementation.

nblumhardt commented 3 years ago

Great! Thanks for all the feedback and ideas @MattMofDoom 👍