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

Rescue errors in alarms report #251

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

elinol
Copy link

@elinol elinol commented Nov 28, 2024

Fix for issue #250

This will rescue any exception when creating alarm report for health checks.

It won't solve the problem where :alarm_handler.get_alarms returns {:error, :bad_module} though.
Any input on that is very welcomed.

@@ -30,6 +30,9 @@ defmodule NervesHubLink.Extensions.Health.DefaultReport do
{"bad alarm term", ""}
end
end
rescue
error ->
%{NervesHubLink.AlarmReportFailed => inspect(error)}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks the Report spec of %{String.t() => String.t()}. But also, I think a simple case here would be simpler and handle situations where a potentially replaced alarm handler does not return expected result

case :alarm_handler.get_alarms() do
  alarms when is_list(alarms) ->
    for {id, description} <- alarms, into: %{}, do: {inspect(id), inspect(description)}
  err ->
    %{"NervesHubLink.AlarmReportFailed" => inspect(err)}
end

There should not be a need to catch inspect/1 calls. If that were to error, it would be a core problem in Elixir and we'd probably want to know.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to this suggestion in the latest commit, and it works fine but the dialyzer is complaining, maybe because it expects :alarm_handler.get_alarms to always return a list?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there needs to be another way. :alarm_handler.get_alarms/0 seems like it's only for experimenting with the default alarm handler. The default alarm handler is missing so many features (deduplication being a really important one) that perhaps it's better to let the user know that they should install a proper alarm handler if they want this information.

{:error, :badmodule} is a successful return value since it indicates that the default alarm handler has been upgraded to a proper one.

There's no generic way of getting the current set of alarms. The only option I can think of is to let the user specify a MFA or function ref to get them. That would also let the user filter alarms if they don't want them all being reported.

Copy link
Author

@elinol elinol Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fhunleth with "let the user specify", would that be via nerveshub web or with a config setting in the nerves project?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should discuss on Slack. The problem is that :alarm_handler.get_alarms/0just won't work, so that function call needs to be removed. However, that probably means we should delete this extension. Either that or make it only available if you're running Alarmist or another alarm handler.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: if someone has their own or another alarm handler they can still integrate it using the custom health report mechanism so they are not stuck.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about moving the alarm handler check to the default report, like this: #263 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. That :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doest hat make this PR irrelevant? I think it might.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants