-
Notifications
You must be signed in to change notification settings - Fork 8
Extended Reporting Service Documentation & Service renaming #146
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
base: v0.x.x
Are you sure you want to change the base?
Extended Reporting Service Documentation & Service renaming #146
Conversation
…al to align with MarketMetering API. * Formatting to align with other Frequenz APIs Signed-off-by: tomni <[email protected]>
Signed-off-by: tomni <[email protected]>
Signed-off-by: tomni <[email protected]>
Signed-off-by: tomni <[email protected]>
* Renamed ReceiveMicrogridElectricalComponentsDataStream RPC * Renamed ReceiveAggregatedMicrogridElectricalComponentsDataStream RPC Signed-off-by: tomni <[email protected]>
* Introduced DownsamplingMethod to ResamplingOptions * Enhanced ResamplingOptions documentation Signed-off-by: tomni <[email protected]>
Signed-off-by: tomni <[email protected]>
Signed-off-by: tomni <[email protected]>
Signed-off-by: tomni <[email protected]>
Signed-off-by: tomni <[email protected]>
…e and ReceiveMicrogridSensorsDataStreamResponse Signed-off-by: tomni <[email protected]>
Signed-off-by: tomni <[email protected]>
Signed-off-by: tomni <[email protected]>
Signed-off-by: tomni <[email protected]>
Signed-off-by: tomni <[email protected]>
Signed-off-by: tomni <[email protected]>
| // algorithm and secret key. All requests to this service must be made | ||
| // over TLS (HTTPS). | ||
| // | ||
| service ReportingService { |
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.
Just FYI, the name change of the service and the RPCs below are considered breaking changes (they don't change the message contents "on the wire" per se, but they will cause older clients to be incompatible). If this is intentional, then it should definitely be mentioned in the release notes.
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.
And also in the PR title. Ideally we could split unproblematic doc updates from breaking changes where possible.
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.
And also in the PR title
Thats done.
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.
then it should definitely be mentioned in the release notes.
OK.
| // Optional; Time window for matching sample interval start timestamps. | ||
| // | ||
| // If omitted, samples from all timestamps are included. | ||
| frequenz.api.common.v1alpha8.types.Interval interval = 1; |
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.
This is also a breaking change, and should be mentioned in the release notes.
| // !!! note "Real-time streams" | ||
| // For open-ended streams, resampling windows advance according to | ||
| // wall-clock time. Window emission is time-driven, not event-driven. | ||
| // As telemetry samples may arrive late (e.g. due to |
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 would suggest mentioning here that there is no fixed guarantee for when a sample may arrive. This seems like a question that would come up from clients.
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've tried to make that a little more precise.
| // !!! note "Resolution constraints" | ||
| // The service enforces an upper bound on the resampling resolution. | ||
| // | ||
| // Resolutions larger than one calendar month are not supported, as |
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 would reword this, because one calendar month is not something that can be expressed in seconds (a 'calendar month' could be anywhere between 28 and 31 days). Maybe "resolutions larger than 30 days"?
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 would even go down to a week, i.e. 7 days. 30 days is not a typical resampling period, but monthly is. However, as Stefan pointed out this would be a new feature and I am not sure if this is in the scope of the resampling of the reporting API. Also, in practice it's just a pandas one-liner on the data.
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.
this would be a new feature and I am not sure if this is in the scope of the resampling of the reporting API. Also, in practice it's just a pandas one-liner on the data.
Right the intention was to allow long term analysis but not needed to receive a lot of data points. But OK, I roll back and limit it.
| // over TLS (HTTPS). | ||
| // | ||
| service ReportingService { | ||
| // Streams metrics for a list of microgrid components. |
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.
Should this also be "electrical components" for consistency?
| // algorithm and secret key. All requests to this service must be made | ||
| // over TLS (HTTPS). | ||
| // | ||
| service ReportingService { |
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.
And also in the PR title. Ideally we could split unproblematic doc updates from breaking changes where possible.
|
|
||
| // Optional UTC end time for the query. | ||
| google.protobuf.Timestamp end_time = 2; | ||
| // Optional; Time window for matching sample interval start timestamps. |
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.
If there is only the interval field left, do we need this message at all?
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.
Well time filter is used in many places and might be extended in the future. If we remove this message we have to duplicate the documentation in many places. You just raised exactly that in the Electricity API.
| // !!! note "Resolution constraints" | ||
| // The service enforces an upper bound on the resampling resolution. | ||
| // | ||
| // Resolutions larger than one calendar month are not supported, as |
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 would even go down to a week, i.e. 7 days. 30 days is not a typical resampling period, but monthly is. However, as Stefan pointed out this would be a new feature and I am not sure if this is in the scope of the resampling of the reporting API. Also, in practice it's just a pandas one-liner on the data.
| // For each interval k: | ||
| // Ik = [T0 + k·ΔT, T0 + (k+1)·ΔT) | ||
| // | ||
| // The aggregated value A_k is by default the arithmetic mean of all |
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.
That's not true, for some metrics there is another aggregation on resampling, e.g. for cumulative energy metrics it is the first value (i.e. the one closest to the time stamp of the window). For some also min or max make sense, but besides bounds I am not sure if we have such in our data off the top of my head.
| // This is the original timestamp of the samples that were aggregated. | ||
| // Semantics depend on whether resampling is enabled: | ||
| // | ||
| // • Without resampling: |
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 don't think we should support formula aggregation without resampling. If we do, we have to specify clearly by which logic the timestamps from independent data sources are aligned before the formula is applied. If they are aggregated without alignment, in most cases the result will not be useful.
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.
True, but only in case metrics from multiple sources are aggregated with a formula. Not true for single source aggregation but I'll adjust it how you suggested it.
…ting API. Signed-off-by: tomni <[email protected]>
This PR clarifies resampling and streaming semantics of the Reporting API. The primary focus of this change is documentation correctness and consistency. Some breaking API changes are introduced.
Key improvements:
sample_timesemantics are clarified:Some schema or wire-format changes were made.