Add sensor ID#169
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds sensor endpoint functionality to the reporting client to support sensor data streaming.
- New protobuf imports and data batch class for sensors have been introduced.
- Two new endpoint methods for listing sensor data (single sensor and multiple sensors) have been implemented.
- Updated release notes to reflect the new sensor endpoint.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/frequenz/client/reporting/_client.py | Added sensor data endpoints, new SensorsDataBatch class, and supporting logic |
| RELEASE_NOTES.md | Updated release notes to include the new sensor endpoint |
Comments suppressed due to low confidence (1)
src/frequenz/client/reporting/_client.py:212
- [nitpick] In the SensorsDataBatch iter method, the field 'component_id' is used to store the sensor id. For clarity, consider renaming it to 'sensor_id' or updating the corresponding docstring so that the intended semantics are unambiguous.
component_id=sid,
| for sensor in self._data_pb.sensors: | ||
| if not sensor.metric_samples and not sensor.states: | ||
| return True | ||
| return False |
There was a problem hiding this comment.
The is_empty method in SensorsDataBatch returns True on the first sensor that lacks both metric_samples and states, which might unintentionally mark a batch as empty even if other sensors contain data. Consider adjusting the logic to check either only the first sensor for consistency with ComponentsDataBatch or verify that all sensors are empty before returning True.
| for sensor in self._data_pb.sensors: | |
| if not sensor.metric_samples and not sensor.states: | |
| return True | |
| return False | |
| return all( | |
| not sensor.metric_samples and not sensor.states | |
| for sensor in self._data_pb.sensors | |
| ) |
There was a problem hiding this comment.
I think you can only check the first entry here.
cwasicki
left a comment
cwasicki
left a comment
There was a problem hiding this comment.
Since we are replicating a lot of code here I would be in favor of refactoring some of our initial code now. If it's very urgent to get in we could also merge this first, but then we should carefully look what we expose now to reduce breaking changes in future.
| for sensor in self._data_pb.sensors: | ||
| if not sensor.metric_samples and not sensor.states: | ||
| return True | ||
| return False |
There was a problem hiding this comment.
I think you can only check the first entry here.
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class SensorsDataBatch: |
There was a problem hiding this comment.
Since this is duplicating a lot of code we have for components, I suggest we make the class more modular so that we can re-use the same code for both, components and sensors.
| retry_strategy=None, | ||
| ) | ||
|
|
||
| receiver = broadcaster.new_receiver() |
There was a problem hiding this comment.
Here we want to return the receiver directly (also for the component counterpart in future).
| filter=stream_filter, | ||
| ) | ||
|
|
||
| def transform_response( |
There was a problem hiding this comment.
I wonder if it is possible to have the flattening loop inside this transform function so that we don't need the private batch function but directly get a receiver of MetricSamples.
There was a problem hiding this comment.
I read up about this a little and would rather like to address it in a separate issue.
| yield data | ||
|
|
||
| # pylint: disable=too-many-arguments | ||
| async def list_single_sensor_data( |
There was a problem hiding this comment.
The list in the method name is outdated since it can also stream now. I think receive would be better.
| microgrid_id: int, | ||
| sensor_id: int, | ||
| metrics: Metric | list[Metric], | ||
| start_dt: datetime | None, |
There was a problem hiding this comment.
These arguments are also different from our other APIs, where we mostly use start/end. However, I guess we will change this soon to {start,end}_time. Better clarify before merging.
There was a problem hiding this comment.
Should I post this in the channel and then open a separate PR based on the consensus?
There was a problem hiding this comment.
We discussed this yesterday and decided to go with {start,end}_time
There was a problem hiding this comment.
Will add it in a small PR!
Signed-off-by: Flora <flora.hofmann@frequenz.com>
flora-hofmann-frequenz
force-pushed
the
sensor_update
branch
from
33a63e7 to
decf7b0
Compare
github-actions
bot
added
the
part:tests
| microgrid_id: int, | ||
| sensor_id: int, | ||
| metrics: Metric | list[Metric], | ||
| start_dt: datetime | None, |
There was a problem hiding this comment.
We discussed this yesterday and decided to go with {start,end}_time
flora-hofmann-frequenz
merged commit f0e0275
into
frequenz-floss:v0.x.x
Implemented comments from #169 for components.
2 participants
No aggregation function added yet.