-
Notifications
You must be signed in to change notification settings - Fork 14
Renamed sampled_at timestamps to origin_time
#345
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
Renamed sampled_at timestamps to origin_time
#345
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR renames the sampled_at timestamp fields to origin_time for consistency across sensor, component, and metric protos.
- Renamed
sampled_atfields toorigin_timein sensor, component, and metric messages - Updated example comments to use
origin_time - Added release note entry for the rename
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| proto/frequenz/api/common/v1/microgrid/sensors/sensors.proto | Renamed sampled_at to origin_time and updated examples |
| proto/frequenz/api/common/v1/microgrid/electrical_components/electrical_components.proto | Renamed sampled_at to origin_time and updated examples |
| proto/frequenz/api/common/v1/metrics/metrics.proto | Renamed sampled_at to origin_time |
| RELEASE_NOTES.md | Added entry for the rename (with a typo) |
Comments suppressed due to low confidence (4)
proto/frequenz/api/common/v1/microgrid/sensors/sensors.proto:172
- [nitpick] Update this comment to reflect the new
origin_timefield name, e.g., "The origin time of this state snapshot."
// The time at which the state was sampled.
proto/frequenz/api/common/v1/microgrid/sensors/sensors.proto:212
- [nitpick] Modify this comment to mention
origin_timeinstead of sampling, e.g., "The UTC origin time of this metric sample."
// The UTC timestamp of when the metric was sampled.
proto/frequenz/api/common/v1/microgrid/electrical_components/electrical_components.proto:478
- [nitpick] Align this comment with
origin_time, for example: "The origin time of the component state snapshot."
// The time at which the state was sampled.
proto/frequenz/api/common/v1/metrics/metrics.proto:188
- [nitpick] Update this comment to reference
origin_time, e.g., "The UTC origin time of when the metric was recorded."
// The UTC timestamp of when the metric was sampled.
9ba6e19 to
ea1f424
Compare
proto/frequenz/api/common/v1/microgrid/electrical_components/electrical_components.proto
Outdated
Show resolved
Hide resolved
llucax
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised about origin_time, I thought and feel for a sample, sample_time feels more natural and obvious, but I guess this was already discussed and decided on, so I will approve.
Nevertheless, I will disable auto-merge just in case to be extra careful.
Signed-off-by: Tiyash Basu <[email protected]>
ea1f424 to
223cf4e
Compare
That was my understanding as well. But it looks like the decision has been changed now. |
|
I also noticed that the timestamp field for communication component states was |
Signed-off-by: Tiyash Basu <[email protected]>
Signed-off-by: Tiyash Basu <[email protected]> # Conflicts: # proto/frequenz/api/common/v1/microgrid/electrical_components/electrical_components.proto
This commit renames the field `snapshot_time` to `origin_time` in the `CommunicationComponentStateSnapshot` message. This makes the field name consistent across `ElectricalComponentStateSnapshot` and `SensorStateSnapshot` messages. Signed-off-by: Tiyash Basu <[email protected]>
223cf4e to
de13b82
Compare
thomas-nicolai-frequenz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
closes #326