elarm functions could return whether the alarm already existed

[hcs42]

We had an idea with Richard that elarm:raise and elarm:clear could return whether they raised/cleared an alarm or did nothing because the alarm was already existing/non-existing.

This way we could write code like this, which logs the problems only once:

case elarm:raise(...) of
    ok ->
        lager:error(...);
    existing ->
        ok
end.

@nygge, what do you think?

[nygge]

An alarm is an indication of an error state in a component/system, so it
should always know if this error state is present or not, therefore it
should never raise the same alarm again if it has already been raised. So I
do not think this makes much sense. Also the alarm_handler in sasl behaves
the same way, set_alarm and clear_alarm both return ok.
On the other hand elarm on a raise, checks if it is a duplicate and if it
is ignores it, so the change is trivial to add this. The reason that elarm
check for duplicates is that some systems/protocols, (e.g. SNMP), may send
duplicates, or messages may be lost, (again SNMP).

On Thu, Jan 30, 2014 at 5:21 AM, Csaba Hoch notifications@github.comwrote:

We had an idea with Richard that elarm:raise and elarm:clear could return
whether they raised/cleared an alarm or did nothing because the alarm was
already existing/non-existing.

This way we could write code like this, which logs the problems only once:

case elarm:raise(...) of
ok ->
lager:error(...);
existing ->
ok
end.

@nygge https://github.com/nygge, what do you think?

Reply to this email directly or view it on GitHubhttps://github.com//issues/9
.

[hcs42]

An alarm is an indication of an error state in a component/system, so it should always know if this error state is present or not, therefore it should never raise the same alarm again if it has already been raised.

That's a reasonable point.

But I thought clearing a non-existent alarm was fine, because the original node_monitor code seemed to do the same thing, clearing the alarm when the node is up, even when starting up. Whole commit here, relevant parts below:

init([Node]) ->
    gproc:add_local_name({wo_node_monitor,Node}),
    {ok, polling, #state{ node = Node }, timer:seconds(1)}.

polling(timeout, #state{ node = Node, wait_1 = W1,
                         wait_2 = W2, max_wait = MaxW } = State) ->
    case try_connect(Node) of
        connected ->
            report_nodeup(Node),
            {next_state, connected, #state{ wait_1 = 0, wait_2 = 1, node = Node }};
        not_connected ->
            Wait = calc_delay(W1,W2,MaxW),
            {next_state, polling, State#state{ wait_1 = W2, wait_2 = Wait }, Wait}
    end.

report_nodeup(Node) ->
    elarm:clear(node_down, Node),
    %% TODO: Report to topology
    io:format("Node ~p is up~n",[Node]).

Do I understand the situation well? Should we change the node monitor?

[nygge]

good catch, node_monitor should not send a clear on start up.

On Thu, Jan 30, 2014 at 11:35 AM, Csaba Hoch notifications@github.comwrote:

An alarm is an indication of an error state in a component/system, so it
should always know if this error state is present or not, therefore it
should never raise the same alarm again if it has already been raised.

That's a reasonable point.

But I thought clearing a non-existent alarm was fine, because the original
node_monitor code seemed to do the same thing, clearing the alarm when
the node is up, even when starting up. Whole commit herehttps://github.com/esl/wombat/commit/1e60d6d805be6cc18e3a51425d8688b93df5239a,
relevant parts below:

init([Node]) ->
gproc:add_local_name({wo_node_monitor,Node}),
{ok, polling, #state{ node = Node }, timer:seconds(1)}.

polling(timeout, #state{ node = Node, wait_1 = W1,
wait_2 = W2, max_wait = MaxW } = State) ->
case try_connect(Node) of
connected ->
report_nodeup(Node),
{next_state, connected, #state{ wait_1 = 0, wait_2 = 1, node = Node }};
not_connected ->
Wait = calc_delay(W1,W2,MaxW),
{next_state, polling, State#state{ wait_1 = W2, wait_2 = Wait }, Wait}
end.

report_nodeup(Node) ->
elarm:clear(node_down, Node),
%% TODO: Report to topology
io:format("Node p is upn",[Node]).

Do I understand the situation well? Should we change the node monitor?

Reply to this email directly or view it on GitHubhttps://github.com//issues/9#issuecomment-33711658
.

[Licenser]

I was looking for this too, and I think it would be helpful. The main reason is that it would prevent keeping the alarm state in two places.

To explain my scenario I send events to folsom to a spiral meter, and if a certain event occurs more then X times over the window of a minute the alarm gets raised. This check is performed periodically. I could store the alarm state in a second place (aside of elarm) and only raise it if it isn't existing but that seems like an extra step to take.

I do see some advantages in not raising a alarm the second time, but then again it would be nicer to return a different value indicating: hey this should not happen!