Add support for filtering devices on alarms #1628
Conversation
| query_current_alarms(product_id) | ||
| |> Repo.all() | ||
| |> Enum.map(fn %{data: data} -> | ||
| Map.keys(data["alarms"]) |
There was a problem hiding this comment.
Currently in this format: Elixir.NervesHubHealth.HealthCheckFailed
We could make them more human friendly perhaps?
There was a problem hiding this comment.
Using inspect/1 on the alarm name will pretty print without Elixir. - We could also add a helper in DeviceHealth.save/1 to preemptively adjust/cast :alarms keys into the prettier format
There was a problem hiding this comment.
I'm wondering if we also remove NervesHubHealth?
There was a problem hiding this comment.
Oh, it's Elixir.NervesHubHealth.HealthCheckFailed actually so solved it by splitting on dots and using the last item.
There was a problem hiding this comment.
Will there be more alarms in the future? We have lots of alarms not explicitly scoped to NervesHubHealth. I'm not sure if that's the only thing expected to be reported here though the name alarms seems to suggest it might
There was a problem hiding this comment.
From what I understand the alarm functionality will be built out, and separated into it's own context/db table. This function will need an update then :)
There was a problem hiding this comment.
NervesHubHealth came from the separate library that we won't be shipping. We can go forward with whatever naming we prefer.
There was a problem hiding this comment.
I agree with using inspect/1 to get it without the Elixir prefix :)
|
Looks alright in dropdown selector: But not when selected: Worth it to fix now or wait for UI update? |
|
Does trimming the Elixir. help with fitting? I don't think we want it anyway. Also, I really like this feature. I'm quite excited about using it. |
| query_current_alarms(product_id) | ||
| |> Repo.all() | ||
| |> Enum.map(fn %{data: data} -> | ||
| Map.keys(data["alarms"]) |
There was a problem hiding this comment.
I'm wondering if we also remove NervesHubHealth?
| """ | ||
| def query_devices_with_alarms() do | ||
| (lr in subquery(latest_row_query())) | ||
| |> from() |
There was a problem hiding this comment.
Yes, otherwise the lr variable is undefined according to compiling errors. It's something with starting the pipes with this lr in.. that I don't like.. that's why the queries aren't pipe-only. But I get why you want them all to be in the same format
lib/nerves_hub/devices/alarms.ex
Outdated
| Selects latest health per device if alarms is populated and device belongs to product. | ||
| """ | ||
| def query_current_alarms(product_id) do | ||
| from( |
There was a problem hiding this comment.
Can you please switch these to pipe queries.
There was a problem hiding this comment.
I can! I found this way easier to follow somehow
|
So the NervesHubHealth prefix for the alarm is entirely up to the thing making the alarm. We can decide if we want a different convention in the default alarms for NervesHubLink as we won't be shipping the NervesHubHealth library. It got folded into NervesHubLink's features implementation. |
Use pipes only in alarm queries
elinol
force-pushed
the
device-alarms
branch
from
3620bd1 to
c16c3ad
Compare
|
|
|
@joshk this should be ready now! |
|
Awesome work @elinol ! |
Adds filtering support for:
Also adds alarm counter on product dashboard with link to filtered devices list.