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

Add metrics filtering to devices query #1598

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

Conversation

elinol
Copy link
Contributor

@elinol elinol commented Oct 11, 2024

Implements filtering on metric values.

What we want is to:
get all devices where the latest associated device metric for specified key is matching the provided filter (greater or less than X)

The current query gives the latest device metric where the requirements are met, so if there are at least one metric report that match for a device, that device will be included in result, no matter the timestamp.

Queries the latest inserted record for given metrics key, and checks if it matches the filter.

Includes a simple little functionality for testing in browser, just add params like ?metric=cpu_temp&metric_operator=gt&metric_value=37.0 to devices URL.

@joshk
Copy link
Collaborator

joshk commented Nov 5, 2024

@elinol did you want a code review on this PR?

@nshoes
Copy link
Contributor

nshoes commented Nov 14, 2024

@elinol bump on @joshk's question! :) Happy to review if this is good to go.

@joshk
Copy link
Collaborator

joshk commented Nov 14, 2024

@elinol oh yeah, I noticed that this is still in Draft, is this ready for review?

Copy link
Collaborator

@joshk joshk left a comment

Choose a reason for hiding this comment

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

Do we need any tests for this?

lib/nerves_hub/devices.ex Outdated Show resolved Hide resolved
@elinol
Copy link
Contributor Author

elinol commented Nov 15, 2024

oh, this has totally been under my radar! Don't think it's ready, will go through it now and mark as ready when done. Thanks @joshk and @nshoes for reminding me

@elinol
Copy link
Contributor Author

elinol commented Nov 15, 2024

@joshk I don't remember why there's no filters selectors added to the UI, did we wanna wait for the new UI? I could add them before merging this, or after if preferred.

@elinol elinol marked this pull request as ready for review November 15, 2024 16:08
@joshk
Copy link
Collaborator

joshk commented Nov 16, 2024

I think it might be good to add it to the current UI, as the new UI is still a little bit away.

@@ -221,10 +225,38 @@ defmodule NervesHub.Devices do
else
query
end

{_, _} ->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this (realized now it could be just a _ ..) instead of cases for {:metrics_key, _} and {:metrics_operator, _} since they depend on :metrics_value to execute anything. Any thoughts on that @joshk ?

@elinol
Copy link
Contributor Author

elinol commented Nov 18, 2024

Before selecting a metric:
image

After selecting the other inputs will display:
image

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.

3 participants