Skip to content
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

Support requesting states without metrics #50

Open
cwasicki opened this issue Sep 16, 2024 · 18 comments
Open

Support requesting states without metrics #50

cwasicki opened this issue Sep 16, 2024 · 18 comments
Labels
part:❓ We need to figure out which part is affected priority:❓ We need to figure out how soon this should be addressed type:enhancement New feature or enhancement visitble to users
Milestone

Comments

@cwasicki
Copy link
Contributor

What's needed?

Currently we need to request at least one metric and receiving states is an include option. This is in line with the current specifications as documented here

// At least one metric must be specified. Failure to do so will result in an empty response.
.

This means that users cannot request states only without any metric or only request states with a dummy metric that will be discarded. It would be good to have the option to request states without requiring to also request metrics.

Proposed solution

At the moment the service accepts requests without metrics and include option for states but doesn't respond with any data (as expected). The simplest option could be to send a response including only the states in this case.

There are other options that require changing the interface, which could improve clarity. FYI @thomas-nicolai-frequenz

Use cases

Users that want to request only states, e.g. for monitoring purpose.

Alternatives and workarounds

No response

Additional context

No response

@cwasicki cwasicki added part:❓ We need to figure out which part is affected priority:❓ We need to figure out how soon this should be addressed type:enhancement New feature or enhancement visitble to users labels Sep 16, 2024
@cwasicki
Copy link
Contributor Author

FYI @sandovalrr

@stefan-brus-frequenz
Copy link
Collaborator

stefan-brus-frequenz commented Sep 17, 2024

The simplest option could be to send a response including only the states in this case.

Agreed, then we would only have to update the docs in the API spec

@thomas-nicolai-frequenz
Copy link

thomas-nicolai-frequenz commented Sep 18, 2024

I do have the following concern about that solution and even it would just require us to update the docs it would feel like a hack. Here are my thoughts:

Using include_options.states within the filter to request states without metrics is not intuitive because include_options is intended to specify additional data to include alongside the primary data specified in the request—in this case, metrics. Using it to request states instead of metrics feels like a backwards compatible hack rather than a proper solution.

diff --git a/reporting.proto b/reporting.proto
index 1234567..89abcde 100644
--- a/reporting.proto
+++ b/reporting.proto
@@ -23,15 +23,17 @@ package frequenz.api.reporting.v1;

 // The Reporting service provides services for real-time and historical metrics monitoring
 // of various microgrid components like Batteries, EV Chargers, and Inverters.
 //
-// Utilize these APIs for tasks ranging from real-time metric streaming to complex,
+// Utilize these APIs for tasks ranging from real-time metric and state streaming to complex,
 // formula-driven historical data aggregation. For detailed information on component
 // categories and the metrics they provide, please consult the referenced component categories
 // imported above.
 //
 service Reporting {
   // Fetches historical metrics for a list of microgrid components.
   //
-  // !!! note
-  //     This RPC endpoint retrieves historical metrics data for components of one or more
-  //     microgrids. Clients need to provide at least one microgrid ID and one component ID
-  //     to get a result.
+  // !!! note
+  //     This RPC endpoint retrieves historical metrics and/or state data for components of one or more
+  //     microgrids. Clients need to provide at least one microgrid ID and one component ID.
+  //     To get a result, clients must specify at least one metric or set `filter.include_options.states` to INCLUDE.
+  //     If neither is provided, the response will be empty.
   rpc ListMicrogridComponentsData(ListMicrogridComponentsDataRequest)
     returns (ListMicrogridComponentsDataResponse) {}

   // [Other RPCs remain unchanged]
@@ -196,15 +198,21 @@ message ListMicrogridComponentsDataRequest {
   //
   // !!! note
   //     In addition to the raw metrics, the API can also return additional information
   //     like errors or operational states of the components during the specified time period.
   //
   message ListMicrogridComponentsDataRequest {
     // General filter criteria for querying microgrid components data.
     message ListFilter {
       // Optional resampling options like resolution for the data, represented in seconds.
       // If omitted, data will be returned in its original representation.
       ResamplingOptions resampling_options = 1;

       // Optional time-based filter criteria.
       TimeFilter time_filter = 2;

       // Include options specifying additional fields to be included in the response.
       IncludeOptions include_options = 3;
     }

     // Encapsulates the microgrid ID and the component IDs within that microgrid for which
     // the historical data should be retrieved.
     //
     // !!! note
     //     Each entry in this repeated field associates a microgrid ID with its respective
     //     component IDs. At least one such association must be provided for a valid request.
     repeated frequenz.api.common.v1.microgrid.MicrogridComponentIDs microgrid_components = 1;

-    // List of metrics to return. Only the specified metrics will be returned.
-    //
-    // !!! note
-    //     At least one metric must be specified. Failure to do so will result in an empty response.
+    // List of metrics to return. Only the specified metrics will be returned.
+    //
+    // !!! note
+    //     Metrics can be omitted if only states are requested.
+    //     At least one of the following must be specified:
+    //     - At least one metric in the `metrics` field.
+    //     - `filter.include_options.states` set to INCLUDE.
+    //     If neither is provided, the response will be empty.
     repeated frequenz.api.common.v1.metrics.Metric metrics = 2;

     // General filter criteria apply to the data retrieval for all specified microgrid components.
     //
     // !!! note
     //     The filter can specify a start time and end time, and/or resampling options.
     //     It can also specify bounds, operational state, or errors to be returned.
     ListFilter filter = 3;

     // Pagination parameters to control the amount of data returned in a single response.
     frequenz.api.common.v1.pagination.PaginationParams pagination_params = 4;
   }

@@ -330,11 +338,13 @@ message IncludeOptions {
   //     Specifies which additional fields should be included in the response.
   //
   message IncludeOptions {
     // Defines whether to include results in the response.
     enum FilterOption {
       FILTER_OPTION_UNSPECIFIED = 0;
       FILTER_OPTION_EXCLUDE = 1;
       FILTER_OPTION_INCLUDE = 2;
     }

     // Optional bound inclusion. By default, bounds are not included in the response.
     optional FilterOption bounds = 1;

     // Optional operational state inclusion. By default, states are not included in the response.
     optional FilterOption states = 2;
   }

@thomas-nicolai-frequenz
Copy link

thomas-nicolai-frequenz commented Sep 18, 2024

If we extend the ListMicrogridComponentsDataRequest to include a new states field, similar to the existing metrics field the user could even request specific states instead of retrieving all of them. Now the challenge is that we'd need to do the same for bounds if someone would just want to receive bounds :-/ That part is not included in the diff below. Its also lacking the alignment of IncludeOptions with the new StateType primary request type proposal below but lets ignore that for now until we've decided which way to go.

diff --git a/reporting.proto b/reporting.proto
index 1234567..89abcde 100644
--- a/reporting.proto
+++ b/reporting.proto
@@ -196,15 +196,32 @@ message ListMicrogridComponentsDataRequest {
   //
   // !!! note
   //     In addition to the raw metrics, the API can also return additional information
   //     like errors or operational states of the components during the specified time period.
   //
   message ListMicrogridComponentsDataRequest {
     // General filter criteria for querying microgrid components data.
     message ListFilter {
       // Optional resampling options like resolution for the data, represented in seconds.
       // If omitted, data will be returned in its original representation.
       ResamplingOptions resampling_options = 1;

       // Optional time-based filter criteria.
       TimeFilter time_filter = 2;

-      // Include options specifying additional fields to be included in the response.
+      // Include options specifying additional fields to be included in the response alongside
+      // the primary data (metrics and states).
       IncludeOptions include_options = 3;
     }

     // Encapsulates the microgrid ID and the component IDs within that microgrid for which
     // the historical data should be retrieved.
     //
     // !!! note
     //     Each entry in this repeated field associates a microgrid ID with its respective
     //     component IDs. At least one such association must be provided for a valid request.
     repeated frequenz.api.common.v1.microgrid.MicrogridComponentIDs microgrid_components = 1;

     // List of metrics to return. Only the specified metrics will be returned.
     //
     // !!! note
-    //     At least one metric must be specified. Failure to do so will result in an empty response.
+    //     Metrics can be omitted if only states are requested.
+    //     At least one of the following must be specified:
+    //     - At least one metric in the `metrics` field.
+    //     - At least one state in the `states` field.
+    //     If neither is provided, the response will be empty.
     repeated frequenz.api.common.v1.metrics.Metric metrics = 2;

+    // List of states to return. Only the specified states will be returned.
+    //
+    // !!! note
+    //     States can be omitted if only metrics are requested.
+    //     At least one of the following must be specified:
+    //     - At least one metric in the `metrics` field.
+    //     - At least one state in the `states` field.
+    //     If neither is provided, the response will be empty.
+    repeated frequenz.api.common.v1.microgrid.components.StateType states = 5;

     // General filter criteria apply to the data retrieval for all specified microgrid components.
     //
     // !!! note
     //     The filter can specify a start time and end time, and/or resampling options.
     //     It can also specify bounds, operational state, or errors to be returned.
     ListFilter filter = 3;

     // Pagination parameters to control the amount of data returned in a single response.
     frequenz.api.common.v1.pagination.PaginationParams pagination_params = 4;
   }

@thomas-nicolai-frequenz
Copy link

thomas-nicolai-frequenz commented Sep 18, 2024

My quick summary would be:

  1. With the first proposal its possible to request all states with or without a metric being provided but its not possible to request just a specific state.
  2. With the second proposal its possible to just request specific states without needing to specify a metric but it would also be possible to request all states alongside a given metric.

Any thoughts? The question is if there is a use case to just request certain states? The good news is that option 2 extends just option 1 if I'm not overlooking anything. So it would be possible to move on with solution 1 and later extend it to solution 2 if its required to just request specific state information. @cwasicki is that matching the requirements?

@thomas-nicolai-frequenz

Is it also a requirement to just request bounds @cwasicki ?

@thomas-nicolai-frequenz
Copy link

thomas-nicolai-frequenz commented Sep 18, 2024

Alright there is a third proposal I very much like in favour of the above. The advantage is a more clear interface and it also supports just receiving certain states. It support request as in the example below:

Example request: Requesting Only Specific States Without Metrics

ListMicrogridComponentsDataRequest {
  microgrid_components: [
    { microgrid_id: 3, component_ids: [301, 302, 303] }
  ],
  metrics: [], // Empty
  states: [COMPONENT_STATE_MAINTENANCE],
  filter: {
    time_filter: { ... }
  },
  include_options: {
    bounds: FILTER_OPTION_EXCLUDE,
    states: FILTER_OPTION_INCLUDE
  },
  pagination_params: { ... }
}

Example request: Requesting Specific States Alongside Metrics

ListMicrogridComponentsDataRequest {
  microgrid_components: [
    { microgrid_id: 2, component_ids: [201, 202, 203] }
  ],
  metrics: [DC_POWER_W],
  states: [COMPONENT_STATE_RUNNING, COMPONENT_STATE_ERROR],
  filter: {
    resampling_options: { ... },
    time_filter: { ... }
  },
  include_options: {
    bounds: FILTER_OPTION_EXCLUDE,
    states: FILTER_OPTION_INCLUDE
  },
  pagination_params: { ... }
}

Changes to the protobuf:

diff --git a/reporting.proto b/reporting.proto
index original..modified 100644
--- a/reporting.proto
+++ b/reporting.proto
@@ -1,8 +1,8 @@
 // protolint:disable MAX_LINE_LENGTH

 // Frequenz Reporting API
 //
 // Copyright 2023 Frequenz Energy-as-a-Service GmbH
 //
 // License:
 // MIT
@@ -23,7 +23,7 @@ syntax = "proto3";

 package frequenz.api.reporting.v1;

import "google/protobuf/timestamp.proto";

 import "frequenz/api/common/v1/metrics/metric_sample.proto";
 import "frequenz/api/common/v1/microgrid/components/components.proto";
@@ -33,7 +33,7 @@ import "frequenz/api/common/v1/pagination/pagination_params.proto";

 // The Reporting service provides services for real-time and historical metrics monitoring
 // of various microgrid components like Batteries, EV Chargers, and Inverters.
 //
-// Utilize these APIs for tasks ranging from real-time metric streaming to complex,
+// Utilize these APIs for tasks ranging from real-time metric and state streaming to complex,
 // formula-driven historical data aggregation. For detailed information on component
 // categories and the metrics they provide, please consult the referenced component categories
 // imported above.
@@ -43,7 +43,7 @@ service Reporting {
     // Fetches historical metrics for a list of microgrid components.

     // !!! note
-    //     This RPC endpoint retrieves historical metrics data for components of one or more
+    //     This RPC endpoint retrieves historical metrics and/or state data for components of one or more
     //     microgrids. Clients need to provide at least one microgrid ID and one component ID
     //     to get a result.
     rpc ListMicrogridComponentsData(ListMicrogridComponentsDataRequest)
@@ -52,7 +52,7 @@ service Reporting {
     // Streams metrics for a list of microgrid components.

     // !!! note
-    //     This RPC endpoint streams metrics data for components of one or more microgrids.
+    //     This RPC endpoint streams metrics and/or state data for components of one or more microgrids.
     //     Clients need to provide at least one microgrid ID and one component ID
     //     to get a result.
     rpc ReceiveMicrogridComponentsDataStream(ReceiveMicrogridComponentsDataStreamRequest)
@@ -193,6 +193,52 @@ message MetricSourceOptions {
     repeated string metric_sources = 2;
 }

+// Message defining the request format for fetching historical metrics, such as electrical
+// measurements, and other information for individual microgrid components.
+//
+// !!! note
+//     In addition to the raw metrics, the API can also return additional information
+//     like errors or operational states of the components during the specified time period.
+//
 message ListMicrogridComponentsDataRequest {
     // Encapsulates the microgrid ID and the component IDs within that microgrid for which
     // the historical data should be retrieved.
     //
     // !!! note
     //     Each entry in this repeated field associates a microgrid ID with its respective
     //     component IDs. At least one such association must be provided for a valid request.
     repeated frequenz.api.common.v1.microgrid.MicrogridComponentIDs microgrid_components = 1;

     // List of metrics to return. Only the specified metrics will be returned.
     //
     // !!! note
-    //     At least one metric must be specified. Failure to do so will result in an empty response.
+    //     Metrics can be omitted if only states or bounds are requested.
+    //     At least one of the following must be specified:
+    //     - At least one metric in the `metrics` field.
+    //     - `include_options.states` set to INCLUDE.
+    //     - `include_options.bounds` set to INCLUDE.
+    //     If none are provided, the response will be empty.
     repeated frequenz.api.common.v1.metrics.Metric metrics = 2;

+    // List of specific states to return. Only the specified states will be returned.
+    //
+    // !!! note
+    //     This field is considered only if `include_options.states` is set to INCLUDE.
+    //     - If `states` field is empty and `include_options.states` is INCLUDE, all states are included.
+    //     - If `states` field is non-empty and `include_options.states` is INCLUDE, only the specified states are included.
+    //     - If `include_options.states` is EXCLUDE, this field is ignored.
+    repeated frequenz.api.common.v1.microgrid.components.StateType states = 5;
+
     // General filter criteria apply to the data retrieval for all specified microgrid components.
     message ListFilter {
         // Optional resampling options like resolution for the data, represented in seconds.
         // If omitted, data will be returned in its original representation.
         ResamplingOptions resampling_options = 1;

         // Optional time-based filter criteria.
         TimeFilter time_filter = 2;

-        // Include options specifying additional fields to be included in the response.
-        IncludeOptions include_options = 3;
+        // [IncludeOptions moved outside of ListFilter]
     }

     // General filter criteria.
     ListFilter filter = 3;

+    // Include options specifying additional data to include in the response
+    // alongside the primary data (metrics).
+    message IncludeOptions {
+        enum FilterOption {
+            FILTER_OPTION_UNSPECIFIED = 0;
+            FILTER_OPTION_EXCLUDE = 1;
+            FILTER_OPTION_INCLUDE = 2;
+        }
+
+        // Optional inclusion of bounds data. Defaults to EXCLUDE.
+        FilterOption bounds = 1 [default = FILTER_OPTION_EXCLUDE];
+
+        // Optional inclusion of states data. Defaults to EXCLUDE.
+        //
+        // - INCLUDE: Include states as specified in the `states` field.
+        //   - If `states` field is empty, include all states.
+        //   - If `states` field is non-empty, include only the specified states.
+        // - EXCLUDE: Do not include any states, regardless of the `states` field.
+        FilterOption states = 2 [default = FILTER_OPTION_EXCLUDE];
+    }
+
+    // Include options for additional data like states and bounds.
+    IncludeOptions include_options = 4;
+
     // Pagination parameters to control the amount of data returned in a single response.
-    frequenz.api.common.v1.pagination.PaginationParams pagination_params = 4;
+    frequenz.api.common.v1.pagination.PaginationParams pagination_params = 6;
 }

 // Response containing historical microgrid component metrics in one or multiple microgrids

...

@stefan-brus-frequenz
Copy link
Collaborator

Now the challenge is that we'd need to do the same for bounds if someone would just want to receive bounds

Aren't bounds tied to metrics anyway? For a battery you might have SoC bounds, power bounds, etc, so you would have to give one or more metrics if you want to read the bounds.

@thomas-nicolai-frequenz

For a battery you might have SoC bounds, power bounds, etc, so you would have to give one or more metrics if you want to read the bounds.

true, valid point. in the above I didn't much focus on bounds tbh.

@thomas-nicolai-frequenz
Copy link

thomas-nicolai-frequenz commented Sep 19, 2024

OK, lets summarise the required use cases:

  • Requesting only metrics
  • Requesting only states (including specific states)
  • Requesting metrics with bounds
  • Requesting metrics with bounds and states

Invalid use cases:

  • Bounds without metrics

@thomas-nicolai-frequenz
Copy link

thomas-nicolai-frequenz commented Sep 19, 2024

Based on the above this should do it:

- // Include options for filtering microgrid components data.
+ // Include options specifying additional data to include in the response
+ // alongside the primary data (metrics).
//
// !!! note
//     Specifies which additional fields should be included in the response.
//
message IncludeOptions {
  // Defines whether to include results in the response message.
  enum FilterOption {
    FILTER_OPTION_UNSPECIFIED = 0;
    FILTER_OPTION_EXCLUDE = 1;
    FILTER_OPTION_INCLUDE = 2;
  }

- // Optional bound inclusion. By default, bounds are not included in the response.
+ // Optional inclusion of bounds data. Defaults to EXCLUDE.
+ //
+ // !!! note
+ //     Bounds can only be included if at least one metric is specified.
- optional FilterOption bounds = 1;
+ optional FilterOption bounds = 1 [default = FILTER_OPTION_EXCLUDE];

-  // Optional operational state inclusion. By default, states are not included in the response.
+  // Optional inclusion of states data. Defaults to EXCLUDE.
+  //
+  // - INCLUDE: Include states as specified in the `states` field.
+  //   - If `states` field is empty, include all states.
+  //   - If `states` field is non-empty, include only the specified states.
+  // - EXCLUDE: Do not include any states, regardless of the `states` field.
-  optional FilterOption states = 2;
+  optional FilterOption states = 2 [default = FILTER_OPTION_EXCLUDE];
}

// ...

// Message defining the request format for fetching historical metrics, such as electrical
// measurements, and other information for individual microgrid components.
//
// !!! note
//     In addition to the raw metrics, the API can also return additional information
//     like errors or operational states of the components during the specified time period.
//
 message ListMicrogridComponentsDataRequest {
     // Encapsulates the microgrid ID and the component IDs within that microgrid for which
     // the historical data should be retrieved.
     //
     // !!! note
     //     Each entry in this repeated field associates a microgrid ID with its respective
     //     component IDs. At least one such association must be provided for a valid request.
     repeated frequenz.api.common.v1.microgrid.MicrogridComponentIDs microgrid_components = 1;

     // List of metrics to return. Only the specified metrics will be returned.
     //
     // !!! note
-    //     At least one metric must be specified. Failure to do so will result in an empty response.
+    //     Metrics can be omitted if only states are requested.
+    //     At least one of the following must be specified:
+    //     - At least one metric in the `metrics` field.
+    //     - `include_options.states` set to INCLUDE.
+    //     If neither is provided, the request is invalid.
+    //
+    //     **Note:** Bounds are associated with metrics. Bounds can only be requested if at least one metric is specified.
     repeated frequenz.api.common.v1.metrics.Metric metrics = 2;

+    // List of specific states to return. Only the specified states will be returned.
+    //
+    // !!! note
+    //     This field is considered only if `include_options.states` is set to INCLUDE.
+    //     - If `states` field is empty and `include_options.states` is INCLUDE, all states are included.
+    //     - If `states` field is non-empty and `include_options.states` is INCLUDE, only the specified states are included.
+    //     - If `include_options.states` is EXCLUDE, this field is ignored.
+    repeated frequenz.api.common.v1.microgrid.components.StateType states = 5;
+
     // General filter criteria apply to the data retrieval for all specified microgrid components.
     message ListFilter {
         // Optional resampling options like resolution for the data, represented in seconds.
         // If omitted, data will be returned in its original representation.
         ResamplingOptions resampling_options = 1;

         // Optional time-based filter criteria.
         TimeFilter time_filter = 2;
     }

     // General filter criteria.
     ListFilter filter = 3;

+    // Include options for additional data like states and bounds.
+    IncludeOptions include_options = 4;

     // Pagination parameters to control the amount of data returned in a single response.
-    frequenz.api.common.v1.pagination.PaginationParams pagination_params = 4;
+    frequenz.api.common.v1.pagination.PaginationParams pagination_params = 6;
 }

@cwasicki
Copy link
Contributor Author

The feature to request only certain state codes seems quite specific to me. I could think that you may want to only get errors though. Or is this covered fully by state COMPONENT_STATE_ERROR?

@thomas-nicolai-frequenz

The feature to request only certain state codes seems quite specific to me.

Indeed it does but keep in mind if you request the states most of the time they will just give you nice aka idle or something. So if someone requests the states I presume they'd be more interested in errors. However, we can skip this feature if not required and introduce it later on. To me its just important that me move into a direction where we could add this feature later one without any major breaking changes.

@thomas-nicolai-frequenz
Copy link

thomas-nicolai-frequenz commented Oct 1, 2024

This will also impact ReceiveMicrogridComponentsDataStreamRequest as IncludeOptions is used in both places. If ListMicrogridComponentsDataRequest where suppose to support requesting metrics for a given component connection also this line from the diff above ...

repeated frequenz.api.common.v1.metrics.Metric metrics = 2;

... would have to be changed to the below and the documentation to be adjusted as shown above for the metrics field:

repeated MetricConnections metrics = 2;

That also means this issue follows the other two: #55 #53

@cwasicki
Copy link
Contributor Author

Invalid use cases:
Bounds without metrics

Following up on this, I realized that requesting bounds without specifying the metric would not make sense, but requesting bounds for a particular metric without getting the values for the metric would indeed be a valid use-case.

@cwasicki cwasicki added this to the v1.0.0 milestone Oct 11, 2024
@thomas-nicolai-frequenz
Copy link

thomas-nicolai-frequenz commented Oct 14, 2024

I realized that requesting bounds without specifying the metric would not make sense, but requesting bounds for a particular metric without getting the values for the metric would indeed be a valid use-case.

OK, so you're suggesting to extend this by adding metric_values to the IncludeOptions filter?

message IncludeOptions {
   optional FilterOption bounds = 1 // default: FILTER_OPTION_EXCLUDE
   optional FilterOption states = 2 // default: FILTER_OPTION_EXCLUDE
   optional FilterOption metric_values = 3 // default: FILTER_OPTION_INCLUDE
}

@cwasicki
Copy link
Contributor Author

Looks good, although I would make the default for metric values INCLUDE.

@stefan-brus-frequenz
Copy link
Collaborator

Note that custom default values only work in proto2 and have been removed in proto3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:❓ We need to figure out which part is affected priority:❓ We need to figure out how soon this should be addressed type:enhancement New feature or enhancement visitble to users
Projects
None yet
Development

No branches or pull requests

3 participants