-
Notifications
You must be signed in to change notification settings - Fork 13
Update to reporting API v1alpha10 #240
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
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 pull request updates the frequenz-client-reporting library to use reporting API v1alpha10 and common API v1alpha8, along with updating to the new metrics package in client-common. The update includes significant changes to import paths, protobuf field names, and the Metric API interface.
- Updates API versions from v1 to v1alpha10 for reporting and v1 to v1alpha8 for common
- Migrates from
frequenz.client.common.metrictofrequenz.client.common.metrics - Updates protobuf field names to match the new API schema (e.g.,
sampled_at→sample_time,states→state_snapshots,component_id→electrical_component_id)
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Updates dependencies to support new API versions (reporting >=0.10.0, common >=0.3.7, typing_extensions >=4.13.0, protobuf >=6.33.1) |
| tests/test_client_reporting.py | Updates import path for ReportingStub to v1alpha10 |
| src/frequenz/client/reporting/cli/main.py | Changes import from metric to metrics module |
| src/frequenz/client/reporting/_types.py | Updates import paths, field names (sampled_at → sample_time, states → state_snapshots, component_id → electrical_component_id), and Metric API usage |
| src/frequenz/client/reporting/_client.py | Updates all import paths, field names, and migrates from Metric.from_proto()/to_proto() to Metric() constructor and .value property |
Comments suppressed due to low confidence (1)
src/frequenz/client/reporting/_types.py:68
- The
is_empty()method checks for the old field name"states"but should check for"state_snapshots"to match the API v1alpha10 update. This inconsistency means the method won't correctly detect state data in the new API version. Line 129 in__iter__correctly uses"state_snapshots", so this should be updated to match.
if not getattr(item, "metric_samples", []) and not getattr(
item, "states", []
):
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com>
Requires also to update `protobuf` and `typing_extension` dependency range. Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com>
This package is compatible with `v1alpha8` of the common API v0.8. Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com>
This includes the major update to common API v1alpha8. Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com>
Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com>
stefan-brus-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.
Not too well versed in pythonics but the PR looks fairly straightforward to me. LGTM
| ts = sample.sample_time.ToDatetime().replace(tzinfo=timezone.utc) | ||
| met = Metric(sample.metric).name |
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.
You can also import from frequenz.client.common.proto and use:
| ts = sample.sample_time.ToDatetime().replace(tzinfo=timezone.utc) | |
| met = Metric(sample.metric).name | |
| ts = datetime_from_proto(sample.sample_time) | |
| met = enum_from_proto(Metric, sample.metric).name |
The datetime conversion is the same, it only avoids the mistake of forgetting the timezone. But when using enum_from_proto() we could support multiple api-common versions in the future, maybe. And it can handle when the proto added a new metric but the client doesn't know about it yet. In this case, your code will raise an exception.
Same for other conversions like these.
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.
Good idea, updated.
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 see you use allow_invalid=False, which also will raise.
I guess it is on purpose, and I see maybe changing it now might not be trivial, but long term I think it would be better to keep clients forward-compatible by allowing the server returning some values we don't know about yet instead of raising an exception.
This will allow users to still get some metric the client doesn't support yet, if for some reason they are stuck with an old client version.
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.
Yes, it's on purpose to have minimal changes. There are long-standing plans of a major rework of the types once the whole microgrid/common updates are final and we go towards v1.
Common conversion helpers might help with future API version updates. Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com>
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.
Other than that and the raising of exceptions (which I understand it might be out of the scope of this PR), LGTM.
| ## Upgrading | ||
|
|
||
| <!-- Here goes notes on how to upgrade from previous versions, including deprecations and what they should be replaced with --> | ||
| * Breaking change to reporting API v1alpha10. |
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.
Maybe it is worth mentioning the switch from metric to metrics, as it involves a few breaking changes too.
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.
The reporting API update includes the major update to common API v1alpha8. This also requires an update to the new metrics package in the client-common.