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.

Sign up for GitHub

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

Jump to bottom

927 status API endpoint and UI #941

Merged
merged 85 commits into from Mar 27, 2024
Merged

927 status API endpoint and UI #941

merged 85 commits into from Mar 27, 2024

Conversation

GustaafL
Copy link
Contributor

@GustaafL GustaafL commented Dec 21, 2023 •
edited by nhoening

Description

API endpoint sensor/<id>/status that will give the status of a sensor. This is checked by confirming data has been posted within an expected time before now (for measurements - forecasts & schedules will be treated next).

Look & Feel

The new page at http://localhost:5000/assets/ID/status/ is the main contribution. It shows a traffic light (green / red) system to indicate which sensor of the asset has "stale" data.

Buttons to reach this page have been added to the account page and asset page (for the shown asset and also for child assets)

How to test

Go to an account page, an asset page or the assets page and click on the "status" button.

Further Improvements

  • Handle staleness for schedules / forecasts
  • Do not show all sensors, maybe only those which have status_specs?
  • Add relevant sensors from flex_context
  • Add caching for faster loads (plus "Check now" button?)

Related Items

Related issue: #927

Flex-context: #1016

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on code under GPL or other license that is incompatible with FlexMeasures

GustaafL added 10 commits December 19, 2023 17:14
feat: add status page url
96ed8d6 
Signed-off-by: GustaafL <guus@seita.nl>
feat: test mock status api request
6a26d86 
Signed-off-by: GustaafL <guus@seita.nl>
feat: test for status with current time
d0303db 
Signed-off-by: GustaafL <guus@seita.nl>
refactor: add prints for testing
54b95fc 
Signed-off-by: GustaafL <guus@seita.nl>
fix: use correct time window for test
6fcd161 
Signed-off-by: GustaafL <guus@seita.nl>
feat: basic UI
2747ef5 
Signed-off-by: GustaafL <guus@seita.nl>
fix: remove accidental change to asset.html
fc68895 
Signed-off-by: GustaafL <guus@seita.nl>
feat: basic UI status page
ea96bd7 
Signed-off-by: GustaafL <guus@seita.nl>
feat: add authentication to status endpoint
a0ee9f1 
Signed-off-by: GustaafL <guus@seita.nl>
fix: change test order to avoid sqlalchemy error
e9904a7 
Signed-off-by: GustaafL <guus@seita.nl>
@GustaafL GustaafL linked an issue Dec 21, 2023 that may be closed by this pull request
Show data connectivity status of asset sensors #927
Closed
GustaafL added 5 commits December 21, 2023 18:01
fix: use python 3.8 syntax
c837563 
Signed-off-by: GustaafL <guus@seita.nl>
fix: test amount of assets for supplier account changed
701b628 
Signed-off-by: GustaafL <guus@seita.nl>
fix: UI buttons clickable and add constants
125101c 
Signed-off-by: GustaafL <guus@seita.nl>
feat: use larger resolution in test to avoid failing chances
a34c849 
Signed-off-by: GustaafL <guus@seita.nl>
docs: add docstrings and descriptions
498e60a 
Signed-off-by: GustaafL <guus@seita.nl>
@GustaafL GustaafL changed the title 927 status API endpoint 927 status API endpoint and UI Dec 22, 2023
GustaafL added 2 commits December 22, 2023 14:38
fix(UI): add id to sensors to fix link to sensor page
69ee0f6 
Signed-off-by: GustaafL <guus@seita.nl>
fix(UI): clean up UI html
83bab76 
Signed-off-by: GustaafL <guus@seita.nl>
@GustaafL
Copy link
Contributor Author

image

Currently the UI shows a list of the last values in the table (not formatted properly yet). Issue #927 describes a tooltip that might show the last values, since there is quite some space it might be better to show them in the table directly.

I would suggest showing a list of the last values and the timestamps in the table. They could also be in a tooltip. @nhoening Please let me know what your preferred option is.

@GustaafL GustaafL marked this pull request as ready for review December 22, 2023 14:00
@nhoening
Copy link
Contributor

Currently the UI shows a list of the last values in the table (not formatted properly yet). Issue #927 describes a tooltip that might show the last values, since there is quite some space it might be better to show them in the table directly.

I would suggest showing a list of the last values and the timestamps in the table. They could also be in a tooltip. @nhoening Please let me know what your preferred option is.

I agree it is interesting to show for now. Let's keep it.
As I said earlier, the page will grow and the extra space might be taken by other things, and then we do not want to show too much information. When we remove it, one can always go to the sensor page and have a look.

nhoening
nhoening requested changes Dec 26, 2023
View reviewed changes
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

Good start, although I recommend not creating a dedicated REST endpoint.

Review was made on a phone, excuses if I overlooked something.

flexmeasures/api/v3_0/sensors.py Outdated Show resolved Hide resolved
flexmeasures/api/v3_0/sensors.py Outdated Show resolved Hide resolved
flexmeasures/api/v3_0/tests/conftest.py Outdated Show resolved Hide resolved
flexmeasures/api/v3_0/tests/test_sensor_data.py Outdated Show resolved Hide resolved
flexmeasures/utils/config_defaults.py Outdated Show resolved Hide resolved
flexmeasures/utils/time_utils.py Outdated Show resolved Hide resolved
flexmeasures/ui/crud/assets.py Outdated Show resolved Hide resolved
flexmeasures/ui/crud/assets.py Outdated Show resolved Hide resolved
feat(test): add test for price sensor status
4516ed1 
Signed-off-by: GustaafL <guus@seita.nl>
@GustaafL
Copy link
Contributor Author

As discussed with @Flix6x:

  • If there are values shown on the status page it's better to show the times of the values than the values themselves.
  • For the status we want to know what the latest possible times for which there should be values are and if those values are there. In the case of posted data this is just the last 4 values but for day ahead prices the status page needs to show if the prices for the next day have been added at the time they are expected. (This might be out of scope and should be a future improvement)
  • The structure, functions and tests should already be made with the above improvements in mind.

I will refactor the code so there is no new endpoint and make a utils function or staticmethod that can be used to query the relevant status data.

feat(test): refactor tests and time functions
8fc6130 
Signed-off-by: GustaafL <guus@seita.nl>
@nhoening nhoening added this to the 0.19.0 milestone Jan 4, 2024
GustaafL added 2 commits January 11, 2024 11:20
feat(status): refactor status functions
5580c44 
Signed-off-by: GustaafL <guus@seita.nl>
fix(tests): remove old tests and conftest setup
29ca558 
Signed-off-by: GustaafL <guus@seita.nl>
@nhoening nhoening mentioned this pull request Mar 17, 2024
Closed
@nrozanov
add child assets sensor statuses to parent asset status page + update…
47a15fe 
… staleness for forecasts

Signed-off-by: Nikolai Rozanov <nickolay.rozanov@gmail.com>
nrozanov and others added 5 commits March 18, 2024 14:50
@nrozanov
merge main
d2e91f3 
Signed-off-by: Nikolai Rozanov <nickolay.rozanov@gmail.com>
@nrozanov
Fix precommit
f664189 
Signed-off-by: Nikolai Rozanov <nickolay.rozanov@gmail.com>
@nrozanov
Fix for forecast status + test get_status handler
0017989 
Signed-off-by: Nikolai Rozanov <nickolay.rozanov@gmail.com>
@nrozanov
Fix dependencies
2b0c2e3 
Signed-off-by: Nikolai Rozanov <nickolay.rozanov@gmail.com>
@nrozanov
Merge branch 'main' into 927-status-page
9ef1ddb
nhoening
nhoening requested changes Mar 19, 2024
View reviewed changes
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

Another good step, thanks for the extra testing.

I have to be a stickler about naming and docstrings, though.

And there is also something I would improve about staleness search when thinking about the existence of forecasts in the sensor data.

flexmeasures/data/services/sensors.py Outdated Show resolved Hide resolved
requirements/3.10/app.txt Outdated Show resolved Hide resolved
flexmeasures/api/common/schemas/tests/test_sensor_data_schema.py Outdated Show resolved Hide resolved
flexmeasures/data/services/sensors.py Outdated Show resolved Hide resolved
flexmeasures/data/services/sensors.py Outdated Show resolved Hide resolved
flexmeasures/data/services/sensors.py Show resolved Hide resolved
flexmeasures/data/services/sensors.py Outdated Show resolved Hide resolved
flexmeasures/data/services/sensors.py Outdated Show resolved Hide resolved
flexmeasures/data/services/sensors.py Outdated Show resolved Hide resolved
flexmeasures/data/services/sensors.py Outdated Show resolved Hide resolved
@nrozanov
Post review fixes + remove forecasting status logic
56b70d5 
Signed-off-by: Nikolai Rozanov <nickolay.rozanov@gmail.com>
@nrozanov
Merge branch '927-status-page' of https://github.com/FlexMeasures/fle…
797346c 
…xmeasures into 927-status-page
@nrozanov
Post review fix
5ea9dac 
Signed-off-by: Nikolai Rozanov <nickolay.rozanov@gmail.com>
nhoening
nhoening requested changes Mar 21, 2024
View reviewed changes
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

Almost there. Two issues in review next to code.
Also, when I look at the UI, namely the asset page, I miss two things:

  • The asset page should also show the status column in the table for its child assets ("All child assets for... ")
  • And it should have a button for its own status. There is already a double button at the top left, not sure how an added "Status" button would look there (or move the content to the right). But let's try.

flexmeasures/data/services/sensors.py Show resolved Hide resolved
requirements/3.8/app.txt Outdated Show resolved Hide resolved
requirements/app.in Outdated Show resolved Hide resolved
@nrozanov
Remove pytest-mock from requirements/app.in
e2f1722 
Signed-off-by: Nikolai Rozanov <nickolay.rozanov@gmail.com>
@nrozanov
Api changes
00b8a58 
Signed-off-by: Nikolai Rozanov <nickolay.rozanov@gmail.com>
nhoening
nhoening approved these changes Mar 24, 2024
View reviewed changes
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

Great thanks!

@nhoening nhoening added this to the 0.21 milestone Mar 24, 2024
@nhoening
conform to our no-trailing-slash policy
03a39f2 
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
@nrozanov nrozanov mentioned this pull request Mar 26, 2024
Open
2 tasks
nhoening and others added 4 commits March 26, 2024 18:02
@nhoening
Merge branch 'main' into 927-status-page
c7c319b
@nrozanov
fix get status test
d9ce151 
Signed-off-by: Nikolai Rozanov <nickolay.rozanov@gmail.com>
@nhoening
Merge branch 'main' into 927-status-page
55c649b
@nrozanov
Update changelog
4ef7e01 
Signed-off-by: Nikolai Rozanov <nickolay.rozanov@gmail.com>
@nhoening nhoening merged commit 94f3321 into main Mar 27, 2024
9 checks passed
@nhoening nhoening deleted the 927-status-page branch March 27, 2024 15:27
Flix6x
Flix6x reviewed Mar 27, 2024
View reviewed changes
Copy link
Contributor

@Flix6x Flix6x left a comment

Choose a reason for hiding this comment

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

Just noticing that the changelog will need a small fix.

documentation/changelog.rst
@@ -9,6 +9,8 @@ v0.21.0 | April XX, 2024
New features
-------------

* Add asset/<id>/status page to view asset statuses [see `PR #41 <https://github.com/FlexMeasures/flexmeasures/pull/941/>`_]
Copy link
Contributor

Choose a reason for hiding this comment

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

PR #941

@Flix6x Flix6x mentioned this pull request Apr 4, 2024
Merged
2 tasks
@nhoening nhoening mentioned this pull request Apr 17, 2024
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Flix6x Flix6x Flix6x left review comments

@nrozanov nrozanov nrozanov left review comments

@nhoening nhoening nhoening approved these changes

Assignees
No one assigned
Labels
Data UI
Projects
None yet
Milestone
0.21.0
Development

Successfully merging this pull request may close these issues.

Show data connectivity status of asset sensors
5 participants
@GustaafL @nhoening @Flix6x @victorgarcia98 @nrozanov