Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] feature: Alert Management and Notifications #1560

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

panaaj
Copy link
Member

@panaaj panaaj commented May 24, 2023

This PR has been REFACTORED to now address #1857.

Linked Issues

Overview

Provide an Alerts mechanism to encompass the management of the lifecycle of alerts and their associated notifications to address when operating conditions become abnormal and measured values fall outside of acceptable thresholds.

It endeavours to align terminology and implement with best practise by adopting the relevant sections of MSC.302(87), A.1021(26) and IEC 68682:2023.

Other references:

Supported Operations

  • Raise alert
  • Fetch Alert details
  • Acknowledge alert
  • Unacknowledge alert
  • Silence alert
  • Update alert metadata / properties
  • Change alert priority
  • Resolve alert
  • Remove alert from Alert List

Alert List

  • Filterable list of alerts
  • Acknowledge ALL alerts
  • Silence ALL alerts
  • Remove ALL resolved alerts from alert list

Notifications

Alerts will emit Notifications throughout their lifecycle to indicate changes in status and when they are resolved.

Features

  • Alerts API (/signalk/v2/api/alerts)
  • Server plugin interface
  • Websocket request support
  • Alerts emit Notifications linked to the alert identifier and containing alert metadata
  • Standard Alerts e.g. Person / man overboard

For raising and clearing Signal K standard alarm types with predefined (overridable)
message text and methods.
@panaaj panaaj requested a review from tkurki May 24, 2023 07:14
@panaaj panaaj added the feature label May 24, 2023
@tkurki
Copy link
Member

tkurki commented May 24, 2023

Sorry - I have a bunch of gripes that I would like to see fixed in V2 notifications:

  • Notifications should have an id (uuid) so that operations/logic, like acknowledgement and clearing notifications can uniquely identify them. This would also induce the API shape: one should be able to GET and operate on the notification by id. In REST that would put the id in the path
  • Imho whatever is raising a notification should not define the notifications method (visual, sound). If there is no way tobe heard method sound has no meaning. I would think the user / person setting up the system should be able to define rules for whatever makes the sound how to act to notifications based on their severity, type and source of the notification. Using v1 method the burden is based on the whatever raises the notification, or the server needs to override it anyway upon reception, which makes having it in the original payload moot. sound and visual are also really really vague.
  • Having the notification type in the path is very v1 hierarchy. I would rather be able to retrieve a list (array) of like objects than an object grouped by type. We could maybe have API paths like notifications/byType/<type> and .../byId/<id>?
  • there are a lot of much more mundane, practical notifications, like over/under voltage/oilPressure/fuel/water etc. Should we cate to those? mob is of course a real thing, but piracy? I guess my point is there is that we definey stuff that nobody uses but don't define practical stuff that would provide value
  • I would like to rename type to severity
  • SK data model has a bunch of device identities. Notifications should have a way to link them to one or more devices. If there is an overvoltage notification there should be a standard way to link it to the specific battery for example.

I like having data, but it should be the same property name in the PUT payload and GET response.

@sbender9 you've dealt a lot more with notifications - thoughts on this?

@panaaj
Copy link
Member Author

panaaj commented May 26, 2023

I don't disagree with your comments, while the intent of the PR was just to address the current alarms detailed in the spec, more than happy to evolve the discussion and the PR here.

@panaaj panaaj changed the title feature: Alarms API to raise & clear Standard Alarm types [WIP] feature: Notifications API Jun 12, 2023
@panaaj
Copy link
Member Author

panaaj commented Jun 12, 2023

In response to the comments above I have updated the inital PR comment to reflect the change in focus from v1 standard alarms to the establishment of a Notifications API that aims to start addressing the concerns raised in @tkurki 's comments e.g. assigning an id which can be used for subsequent actions.

In it's current form it can be considered a skeleton API profile (for interactive / client use) and interface method(s) (for plugins and data stream handlers) to raise, update and clear notifications in a consistent way.

There is still much to do and also scenarios that have not yet been addressed, e.g. the handling of notifications originating from within Signal K deltas generated by external devices / sensors.

@preeve9534
Copy link

preeve9534 commented Oct 18, 2023

I’ve been doing some work with Notifications recently which has prompted some thoughts...

  1. Acknowledging a notification. So what are the semantics and purpose of the ACK discussed in the proposal?

    I have a two channel (beacon and buzzer) annunciator plugin which allows the user to suppress (acknowledge?) channels independently - the obvious solution to this is to tag involved notifications as being suppressed on a particular channel: hence ACK-BEACON, ACK-BUZZER (or much less desirably ACK’ and ‘BEACON’ together).

    I think that ACK as discussed is a specific case of a more generally useful mechanism that would allow a notification to be tagged. An API for deleting a tag would also be required.

    This would also imply the enquiry /notifications/byTag/<tagname> (but see (3) below).

  2. Scope of application. Just to note that the proposed features are likely only valuable if they apply to all notifications (not just the system subset). I have never used a system notification on my ship, but use tank, electrical and sensor notification events regularly.

  3. List or object results? The proposal shows object (or associative array results) and refers to them as a list. I'll keep doing that...

  4. Why have two list result organisations (key by path and key by id)? I would favour all list APIs (byType, byTag, byPath (see below)) returning a single organisation - object indexed by id would be my preference. This would mean that /notifications should return an object indexed by id.

  5. I have found myself wanting to recover all notifications within a particular scope (as represented by a partial path or path prefix), so the API /notifications/byPath/<path-prefix> makes sense to me - I could then recover a dict of all notifications on tanks (or whatever).

P

@panaaj
Copy link
Member Author

panaaj commented Oct 18, 2023

  1. Acknowledging a notification. So what are the semantics and purpose of the ACK discussed in the proposal?

At this time, the working position is based on a fairly common notification / alarm lifecycle (well in my experience at least) where there are sources which can raise different types of alarms and there are "many eyes on glass" to take action as required.

  1. Alarm raised
  • (optionally) Alarm silenced (still not actioned)
  1. Alarm Acknowledged (action being taken)
  2. Alarm Cleared (remediating action completed)

Silencing the alarm is an "Action" but may not apply globally to the alarm but just to the console where the silence action was taken. There may be other actions that fall into a similar basket.

There is clearly a need to agree the alarm / notification lifecycle to ensure the API and operation is fit for purpose and not unnecessarily complex.

@panaaj
Copy link
Member Author

panaaj commented Oct 18, 2023

2. Scope of application. Just to note that the proposed features are likely only valuable if they apply to all notifications (not just the system subset). I

Absolutely the scope is all notifications to be actionable regardless of the source.

@panaaj
Copy link
Member Author

panaaj commented Oct 18, 2023

3. List or object results?

This aligns with the current format of how of entries are returned in Signal K.

@panaaj
Copy link
Member Author

panaaj commented Oct 19, 2023

4. Why have two list result organisations (key by path and key by id)? I would favour all list APIs (byType, byTag, byPath (see below)) returning a single organisation - object indexed by id would be my preference. This would mean that /notifications should return an object indexed by id.

The current approach is a rudimentary way to add notification ids and be backwards compatable with current operation. I expect that the underlying notification system will need to re-engineered as it will need to function quite differently to the way it does today (where they are effectively just another path).

@preeve9534
Copy link

@panaaj - thank you for explaining current thinking on this.

On the alarm life cycle front I worry about conflating the semantics of
notifications with those of alarms.

It even jars when I see the expression "Standard Alarms" which in my
view should be "Standard Notifications" (which mostly, I guess may well
be alarming and their appearance probably would warrant an actual alarm).

In my head the only Notification lifecycle events are create and destroy.

I acknowledge, of course, that the obvious place to capture some alarm
related information is in a Notification object but I think this holds true
more generally for other Notification-related activities (see below).

Thus, I remain concerned about reifying these somewhat-notification-related
operations into specific Notification API calls like:

HTTP PUT 'http://hostname:3000/signalk/v2/api/notifications/ack/ac3a3b2d-07e8-4f25-92bc-98e7c92f7f1a'

when, debatably, they have nothing to do with a Notifcation per-se.

Conditions like ACK and SILENCE are (to me) alarm related concepts.

I have a plugin that forwards Notifications to the WAN by email and web-push and it is
useful to note that this action has occurred in the source Notification, so
EMAILED and PUSHED seem, in the same way, to be equally valid notions.

You will see how I got to the more general proposal of a tag for flagging things that,
in my head, are not notification specific.

P

@tkurki
Copy link
Member

tkurki commented Dec 27, 2024

Related OpenCPN discussion OpenCPN/OpenCPN#4253

@mairas
Copy link
Contributor

mairas commented Dec 29, 2024

I added a data model study I wrote as a separate Issue #1857. In my opinion, notifications and alerts should be separate entities because their lifecycle and treatment are different. We could either refactor this work to deal with alerts or reduce its scope to only address notifications (that in my opinion are fleeting messages for which operator action is not expected).

@panaaj
Copy link
Member Author

panaaj commented Jan 4, 2025

I have pushed an update to create a skeleton alert manager and alert class that aligns to the lifecycle in #1857 to provide a basis for building out an approach.
It implements an HTTP API for proving out operation and some basic documentation.

@panaaj panaaj changed the title [WIP] feature: Notifications API [WIP] feature: Alert Management and Notifications Jan 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants