Refactor: remove kits - Githubissues

timcowlishaw commented 11 months ago

Work in progress on #241 - this removes kits and makes devices relate directly to sensors via components.

The migration is probably a good place to start to work out what's going on here at a high level.

Still TODO:

~See if we can remove OrphanDevices too~

timcowlishaw commented 10 months ago

Fixed the following TODO items:

Add logic to create components on a device when a reading is received for a new sensor.
Decide what to do re: order of columns in the device CSV archive (the one currently failing test relates to this)
Quitar spurious dates (recorded_at and added_at on device)
Test RawStorer / Legacy Create (is this still needed?)

Regarding the dates - I've (for now) left the added_at field on the "/user/:id" and "/devices/world_map" endpoints as, in the first case, "added_at" is probably a better name for this property on the user (they already have their own created_at date), and in the case of the world map, we need to check that the JS doesn't expect it to be there. It's removed elsewhere though.

timcowlishaw commented 10 months ago

Hey @oscgonfer - I think this is now ready for a really thorough test on staging, and then a merge! We'll go through it on monday afternoon - have a good weekend in the meantime!

timcowlishaw commented 10 months ago

Note - this will have to be release along with an update to fablabbcn/smartcitizen-web to remove references to the kits endpoint. We should check also that onboarding works against this branch, which i will do locally and report back

timcowlishaw commented 10 months ago

TODO: a bit more tidying up of dates, while we're here:

last_recorded_at on Device should become last_reading_at everywhere
User joined_at should return to just being created_at for consistency.

oscgonfer commented 10 months ago

Hi there! Will running the db:migrate seems like there is a small issue, I believe coming from db/migrate/20230704150532_refactor_kits.rb:109 :

110               INSERT INTO components
111               (device_id, sensor_id, key, created_at, updated_at)
112               VALUES (?, ?, ?, ?, ?)
113             """, [device_id, sensor_id, key, created_at, updated_at])

Raising:

PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "components_pkey"
DETAIL:  Key (id)=(81) already exists.

oscgonfer commented 10 months ago

Also, after checking the new structure, I believe we should have an additional column on the components table:

Conceptually, it will represent the idea of location or sensor bus (type integer) which will defaulted as 1, but that could be any number. The idea behind this is to prepare the data model to have the possibility to have more than one distinguishable component on a device with the same sensor_id.

We will then have to write a parser on mqtt-task to be able to convert what the kit sends (no longer it would be just a sensor_id, but a combined value between sensor_id and sensor_location, whenever it has more than one sensor with the same sensor_id, but in different data buses. For now, we can default the sensor_location to 0 in case we don't receive anything, but that way we set the scene later on.

timcowlishaw commented 10 months ago

Hi there! Will running the db:migrate seems like there is a small issue, I believe coming from db/migrate/20230704150532_refactor_kits.rb:109 :

110               INSERT INTO components
111               (device_id, sensor_id, key, created_at, updated_at)
112               VALUES (?, ?, ?, ?, ?)
113             """, [device_id, sensor_id, key, created_at, updated_at])

Raising:

PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "components_pkey"
DETAIL:  Key (id)=(81) already exists.

Hey Oscar!

Aha, this is because of the primary key sequences getting out of sync when we restore from a backup, you'll need to run

ActiveRecord::Base.connection.tables.each do |t|
  ActiveRecord::Base.connection.reset_pk_sequence!(t)
end

in the rails console first (now we've deleted the weird devices, it shouldn't cause a problem).

This shouldn't need to be done in production, it's only because we're restoring from a backup that it's an issue.

timcowlishaw commented 10 months ago

(btw i've also pushed a small commit to get rid of the require "pry" statements that we didn't need and which were causing a seperate issue)

oscgonfer commented 10 months ago

Hi there! Will running the db:migrate seems like there is a small issue, I believe coming from db/migrate/20230704150532_refactor_kits.rb:109 :
110               INSERT INTO components
111               (device_id, sensor_id, key, created_at, updated_at)
112               VALUES (?, ?, ?, ?, ?)
113             """, [device_id, sensor_id, key, created_at, updated_at])
Raising:
PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "components_pkey"
DETAIL:  Key (id)=(81) already exists.
Hey Oscar!

Aha, this is because of the primary key sequences getting out of sync when we restore from a backup, you'll need to run
ActiveRecord::Base.connection.tables.each do |t|
  ActiveRecord::Base.connection.reset_pk_sequence!(t)
end
in the rails console first (now we've deleted the weird devices, it shouldn't cause a problem).

This shouldn't need to be done in production, it's only because we're restoring from a backup that it's an issue.

Aha!! OK! that did it! Great work! :clap:

Will test it a bit and come back to you. I will put it on a separate comment for traceability of TO-DOs

oscgonfer commented 10 months ago

Keeping track here of things @timcowlishaw

[x] Remove raw_value and prev_raw_value from sensor fields in device
- [x] Add timestamp of the actual measurement for in replacement of raw_value and prev_raw_value for latest reading
[x] Additional field in component as described here https://github.com/fablabbcn/smartcitizen-api/pull/246#issuecomment-1733326225
[x] User joined_at should return to just being created_at for consistency (https://github.com/fablabbcn/smartcitizen-api/pull/246#issuecomment-1723529390)
[x] Add a way to populate the device.description based on hardware info... (this can be problematic, because the description can be changed in the device>edit view in the front-end, but let's talk about it)

timcowlishaw commented 10 months ago

Aha thanks @oscgonfer that's super helpful. I'll get all that sorted this week!

timcowlishaw commented 9 months ago

Hey @oscgonfer , I've just pushed a commit with the following changes:

Add 'location' column to components.
Remove user joined_at alias, revert to created_at for consistency
Rename devices last_recorded_at column to last_reading_at, and remove the alias, for consistency.
remove 'raw_value' and 'raw_prev_value' from the data exposed for devices.

I think it'd be good to have a chat about "adding the timestamp of the actual measurement" and "populating the device description" next week if poss - there's a few things there i'm not clear on!

oscgonfer commented 9 months ago

Hey @oscgonfer , I've just pushed a commit with the following changes:
* Add 'location' column to components.

* Remove user joined_at alias, revert to created_at for consistency

* Rename devices last_recorded_at column to last_reading_at, and remove the alias, for consistency.

* remove 'raw_value' and 'raw_prev_value' from the data exposed for devices.
I think it'd be good to have a chat about "adding the timestamp of the actual measurement" and "populating the device description" next week if poss - there's a few things there i'm not clear on!

Thanks @timcowlishaw ! Let's talk about these points in person.

Only one comment: we should think about the location name (and possible confusions that derive from that). Maybe we can give it a thought (bus... or something similar?)

oscgonfer commented 9 months ago

Regarding the timestamp, let's take two payloads of the same device:

{
    t:2023-10-04T09:39:55Z,
    10:99,
    14:256,
    55:16.12,
    56:65.84,
    53:47.51,
   58:102.08
}

{
    t:2023-10-04T09:40:55Z,
   10:99,
   14:256
}

The second payload has data for only two sensors. But on the device endpoint we have no way to know when the last actual data of a sensor took place, as it is all associated only with last_reading_at

timcowlishaw commented 9 months ago

Added migration to rename 'location' to 'bus' - now just pending the work on per-component last reading times, which i'll get to after the remaining bugfixes elsewhere!

timcowlishaw commented 9 months ago

I've also added per-component last_reading_at timestamps, updated with each new reading!

timcowlishaw commented 9 months ago

BTW @oscgonfer when this refactor is done and merged, I'd like to start thinking about refactoring and improving the test coverage for the data storers and MQTT handler - they're extremely "enmarañados" at the moment and with quite flaky coverage, which is making me very nervous about changing anything to do with them, and they're a pretty key part of the system. We can discuss next time we see each other!

oscgonfer commented 9 months ago

I've also added per-component last_reading_at timestamps, updated with each new reading!

I am so happy about this one!

:heart:

timcowlishaw commented 8 months ago

Squashed and rebased onto master!

timcowlishaw commented 8 months ago

@oscgonfer re-rebased and all tests passing!

oscgonfer commented 8 months ago

Hi @timcowlishaw

I was testing this.

Seems like component creation is working as expected. However, I get a 500 when trying to query readings data. Logs of app container below:

smartcitizen-api-app-1  | F, [2023-11-22T16:04:13.597489 #1] FATAL -- : [91.126.198.94] [653c777c-aaaf-4b5e-bff6-797e929f39ae]   
smartcitizen-api-app-1  | [91.126.198.94] [653c777c-aaaf-4b5e-bff6-797e929f39ae] NoMethodError (undefined method `find_component_by_sensor_id' for #<Device id: 16602,...
...
smartcitizen-api-app-1  | Did you mean?  find_or_create_component_by_sensor_id):

Sent it to sentry for more details:

https://iaac.sentry.io/share/issue/bb0748b4eddd421cada3a389a9c904ce/

timcowlishaw commented 8 months ago

Hey @oscgonfer - fyi, i've just pushed a commit to fix that error, it was an oversight of mine.

timcowlishaw commented 7 months ago

Hey @oscgonfer, I've` added a commit to address #284, but it might be good to have a quick chat about it before we merge, to make sure i've understood correctly what should be happening!

I've also rebased off the docker-compose update branch - it won't matter though as long as we merge the docker updates first, which seems sensible. If we can't do that, I can unpick it with an interactive rebase no problem

oscgonfer commented 7 months ago

Adding some comments here based on our conversation in slack. We need to ensure that the components have unique sensor keys to avoid issues in the future. Since we are not restraining the kits anymore, there could be:

devices that have one or more sensors that are the same;
devices that have sensors in which the key is the same

On creating the component, then we should make sure that the key is unique. The same concept can be added to the migration.

timcowlishaw commented 4 months ago

Ok! I've removed the hardware_description, made sure caching is working on world_map, and also made sure we never show the authorized serialisation of devices within it

oscgonfer commented 4 months ago

Hi @timcowlishaw

I was checking this, and I found out one potential issue with the default_keys.

This device (16599) is a test device in which I am sending random MQTT payloads. It turns out, that not all sensors in that device have default_keys. In that particular device, out of many sensors (total 225), the following sensor ids have no default_key (53):

[(132, None),
 (137, None),
 (142, None),
 (147, None),
 (213, None),
 (214, None),
 (215, None),
 (216, None),
 (111, None),
 (47, None),
 (44, None),
 (163, None),
 (222, None),
 (173, None),
 (63, None),
 (37, None),
 (3, None),
 (176, None),
 (78, None),
 (124, None),
 (19, None),
 (52, None),
 (57, None),
 (82, None),
 (81, None),
 (20, None),
 (60, None),
 (219, None),
 (178, None),
 (66, None),
 (114, None),
 (117, None),
 (116, None),
 (115, None),
 (70, None),
 (86, None),
 (74, None),
 (162, None),
 (59, None),
 (69, None),
 (226, None),
 (227, None),
 (192, None),
 (209, None),
 (54, None),
 (223, None),
 (225, None),
 (224, None),
 (181, None),
 (217, None),
 (218, None),
 (90, None),
 (171, None)]

Taking the readings from the device, for instance of sensor 215, I see data, but, the key is a bit weird (see https://staging-api.smartcitizen.me/v0/devices/16599/readings?sensor_id=215&rollup=5s&from=2024-03-05T16:42:00Z) json head:

{
    "device_id": 16599,
    "sensor_key": "_61",
    "sensor_id": 215,
    "component_id": 76689,
    "rollup":" 5s",
    "function": "avg",
    "from": "2024-03-05T16:42:00Z",
    "to":" 2024-03-06T17:42:02Z",
    "sample_size": 495,
    ...
}

Which should be no problem for this particular device, as the data seems to always be unequivocally there, but the sensor_key will not be reused. I assume that the fact that sensor_key=_61 comes from the concatenation of None + _[int] which happens several times across that device in particular. This would make the possibility to check for that particular sensor across different devices not feasible through the key, but only with the id (again, not an issue in itself, but I don't think this is a we planned it).

timcowlishaw commented 4 months ago

Hi @timcowlishaw

I was checking this, and I found out one potential issue with the default_keys.

We never made a separate issue for this, so just noting here that we fixed this with the change to the default_keys logic

oscgonfer commented 4 months ago

Hi @timcowlishaw I was checking this, and I found out one potential issue with the default_keys.

We never made a separate issue for this, so just noting here that we fixed this with the change to the default_keys logic

Yes, sorry! Thank you!

fablabbcn / smartcitizen-api

Refactor: remove kits #246