Extend the dispatch API to allow filtering of reoccurring and non-reoccurring dispatches by their end time

[thomas-nicolai-frequenz]

What's needed?

Can the filtering be extended to allow filtering by end time and duration in order to determine if a dispatch has finished or not?

Proposed solution

index 4166925..6546d5f 100644
--- a/proto/frequenz/api/dispatch/v1/dispatch.proto
+++ b/proto/frequenz/api/dispatch/v1/dispatch.proto
@@ -149,6 +149,18 @@ message DispatchDetail {
   google.protobuf.Timestamp modification_time = 4;
 }
+ // Enum for the type of dispatch based on its recurrence.
+ enum RecurrenceType {
+   // UNSPECIFIED: Recurrence type is not specified.
+   RECURRENCE_TYPE_UNSPECIFIED = 0;

+   // NON_RECURRING: Dispatches that do not recur.
+   RECURRENCE_TYPE_NON_RECURRING = 1;

+   // RECURRING: Dispatches that recur.
+   RECURRENCE_TYPE_RECURRING = 2;               
+ }

// Filter parameter for specifying multiple time intervals
message TimeIntervalFilter {
-  // Filter by start_time >= this timestamp
+  // Filter by time >= this timestamp.
-  google.protobuf.Timestamp start_from = 1;
+  google.protobuf.Timestamp from = 1;

-  // Filter by start_time < this timestamp
+  // Filter by time < this timestamp.
-  google.protobuf.Timestamp start_to = 2;
+  google.protobuf.Timestamp to = 2;

-  // Filter by recurrence.end_criteria.until >= this timestamp
-  google.protobuf.Timestamp end_from = 3;

-  // Filter by recurrence.end_criteria.until < this timestamp
-  google.protobuf.Timestamp end_to = 4;
}

// Parameters for filtering the dispatch list
message DispatchFilter {
-  // Filter by component ID or category
+  // Optional filter by component ID or category.
  repeated ComponentSelector selectors = 1;

-  // Filter by time interval
-  // If no interval is provided, all dispatches starting from the
-  // current timestamp will be included.
-  TimeIntervalFilter time_interval = 2;

-  // Filter by active status
+  // Optional filter by active status.
  // If this field is not set, dispatches of any active status will be included.
-  optional bool is_active = 3;
+  optional bool is_active = 2;

-  // Filter by dry run status
+  // Optional filter by dry run status.
  // If this field is not set, dispatches of any dry run status will be included.
-  optional bool is_dry_run = 4;
+  optional bool is_dry_run = 3;

+  // Optional filter by reoccurring and none-reoccurring dispatches.
+ // If not specified both types will be returned.
+  optional RecurrenceType recurrence_type = 4;

+  // Optional filter by start time.
+  // If no interval is provided, all dispatches will be returned.
+  TimeIntervalFilter start_time_interval = 5;

+  // Optional filter by end time
+  // Filter dispatches based on their explicit or estimated end time.
+  TimeIntervalFilter end_time_interval = 6;

+  // Optional filter by update time
+  TimeIntervalFilter update_time_interval = 7;
}

Use cases

No response

Alternatives and workarounds

No response

Additional context

No response

[thomas-nicolai-frequenz]

Note: If no start_time filter is provided it should return all dispatches.

[Marenz]

I would suggest to just remove RecurrenceType and just use optional bool is_recurring

[thomas-nicolai-frequenz]

I would suggest to just remove RecurrenceType and just use optional bool is_recurring

We can do but this would eliminate any chance of extending this in the future.

[Marenz]

What possible extension could there be, either it's recurring or not, and we can always add a new field for more options

[llucax]

If we add more ways to filter in the future based on recurrence specifics, then probably users will have to specify is_recurring and the extra rule, or things will get confusing. A way to declare this in a way that's easier to extend and still keeps all recurrence filtering together could be a oneof, for example:

message DispatchFilter {
  // ...
  oneof recurrence {
    bool is_recurring = 4;
  }

It is a bit more verbose to use at first (filter.recurrence.is_recurring) but we can easily extend in the future by adding more ways to filter by recurrence rules by adding more options to the oneof.

[thomas-nicolai-frequenz]

I'm not a big fan of oneofs. I'd like to avoid them as much as possible tbh.

[Marenz]

after thinking a bit about this one, I think it might actually be best to do something like this:

message DisptachFilter {
  message RecurrenceFilter {
  // Left empty until we decide for what filters we want exactly
  }
  [..]

  RecurrenceFilter recurrence_filter = 4;
  [..]
}

That way recurrence_filter can be either None or the (so far empty) RecurrenceFilter object/message. And we can fill that message with filters as we like without breaking backwards compatibility.

[llucax]

I'm not a big fan of oneofs. I'd like to avoid them as much as possible tbh.

Why?

[Marenz]

More detailed example:

  message RecurrenceFilter {
    // Filter by recurring status
    // True: Only recurring dispatches will be returned.
    // False: Only non-recurring dispatches will be returned.
    // Unset: Both recurring and non-recurring dispatches will be returned, 
    //        recurring dispatches will be filtered based on the recurrence
    //        filter, while non-recurring dispatches will be returned regardless.
    // Examples: 
    // - To retrieve only recurring dispatches:
    //   filter { recurrence_filter { is_recurring: true } }
    // - To retrieve only non-recurring dispatches:
    //   filter { recurrence_filter { is_recurring: false } }
    // - To retrieve all dispatches:
    //   filter { recurrence_filter {} }
    // - To retrieve only recurring dispatches with a specific frequency:
    //   filter { recurrence_filter { is_recurring: true, freq: [FREQUENCY_DAILY] } }
    // - To retrieve only recurring dispatches with a specific frequency but
    //   also include non-recurring dispatches:
    //   filter { recurrence_filter { freq: [FREQUENCY_DAILY] } }
    optional bool is_recurring = 1;

    // Filter by frequency
    // If not specified, all frequencies will be returned.
    repeated RecurrenceRule.Frequency freq = 2;
  }

[Marenz]

Updated https://github.com/frequenz-floss/frequenz-api-dispatch/pull/184 accordingly

[thomas-nicolai-frequenz]

I'm not a big fan of oneofs. I'd like to avoid them as much as possible tbh.

Not all client implementations support them. Its needs additional code on the client side too.

[Marenz]

We already use oneof in the ComponentSelector, if the client doesn't support it, the whole API is kind of unusable to them...

[thomas-nicolai-frequenz]

We already use oneof in the ComponentSelector, if the client doesn't support it

Sure, that doesn't mean its better to use it everywhere and I'm also not sure its really required in this case. I'd rather go for the boolean approach instead.

[Marenz]

The latest suggestion doesn't include oneof anymore, still waiting on your feedback for that one :)

[thomas-nicolai-frequenz]

still waiting on your feedback for that one :)

oh sorry I thought you just kept going with this. I like it.

[llucax]

Not all client implementations support them. Its needs additional code on the client side too.

Really? Which ones? What do you mean by it needs additional code on the client side?

I really like oneof, I think it makes message better typed, instead of having 3 different fields and then you need to validate that if one is present some other shouldn't be present is a mess. oneof are type unions in programming languages (both rust and python support it via T1 | T2).

For example, with the current proposal:

  message RecurrenceFilter {
    optional bool is_recurring = 1;
    repeated RecurrenceRule.Frequency freq = 2;
  }

What happens if I get is_recurrence == false but also a freq. Which one should I follow? I think exactly for this case oneof is great.

[Marenz]

Sorry, the PR for these changes already got merged even though the discussion is ongoing, but we can just add the changes in a new PR.

[Marenz]

Here is an updated proposal for the DispatchFilter definition incorporating Luca's feedback:

message DispatchFilter {
  message RecurrenceFilter {
    // The frequency specifier of this recurring dispatch
    optional RecurrenceRule.Frequency freq = 1;

    // How often this dispatch should recur, based on the frequency
    // Example:
    // - Every 2 hours:
    //   freq = FREQUENCY_HOURLY
    //   interval = 2
    // Valid values typically range between 1 and 10_000, depending on the implementation.
    optional uint32 interval = 2;

    // When this dispatch should end.
    // A dispatch can either recur a fixed number of times, or until a given timestamp.
    // If this field is not set, the dispatch will recur indefinitely.
    optional EndCriteria end_criteria = 3;

    // On which minute(s) of the hour does the event occur
    repeated uint32 byminutes = 4;

    // On which hour(s) of the day does the event occur
    repeated uint32 byhours = 5;

    // On which day(s) of the week does the event occur
    repeated RecurrenceRule.Weekday byweekdays = 6;

    // On which day(s) of the month does the event occur. Valid values are 1 to 31 or -31 to -1.
    //
    // For example, -10 represents the tenth to the last day of the month.
    // The bymonthdays rule part MUST NOT be specified when the FREQ rule part is set to WEEKLY.
    repeated int32 bymonthdays = 7;

    // On which month(s) of the year does the event occur
    repeated uint32 bymonths = 8;
  }

  // Other existing filters...
  oneof recurrence {
    // Filter by recurring status
    // True: Only recurring dispatches will be returned.
    // False: Only non-recurring dispatches will be returned.
    // Examples: 
    // - To retrieve only recurring dispatches:
    //   filter { recurrence { is_recurring: true } }
    // - To retrieve only non-recurring dispatches:
    //   filter { recurrence { is_recurring: false } }
    // - To retrieve all dispatches:
    //   filter { recurrence {} }
    optional bool is_recurring = 4;

    // Filter by recurrence details
    // Examples: 
    // - To retrieve only recurring dispatches with a specific frequency:
    //   filter { recurrence { filter { freq: FREQUENCY_DAILY } } }
    // - To retrieve recurring dispatches with a specific frequency and interval:
    //   filter { recurrence { filter { freq: FREQUENCY_HOURLY, interval: 2 } } }
    // - To retrieve recurring dispatches that end after a specific criteria:
    //   filter { recurrence { filter { end_criteria: { count: 10 } } } }
    // - To retrieve recurring dispatches at specific minutes of the hour:
    //   filter { recurrence { filter { byminutes: [0, 15, 30, 45] } } }
    // - To retrieve recurring dispatches at specific hours of the day:
    //   filter { recurrence { filter { byhours: [8, 12, 16] } } }
    // - To retrieve recurring dispatches on specific days of the week:
    //   filter { recurrence { filter { byweekdays: [WEEKDAY_MONDAY, WEEKDAY_WEDNESDAY] } } }
    // - To retrieve recurring dispatches on specific days of the month:
    //   filter { recurrence { filter { bymonthdays: [1, 15, 30, -1] } } }
    // - To retrieve recurring dispatches in specific months of the year:
    //   filter { recurrence { filter { bymonths: [1, 6, 12] } } }
    RecurrenceFilter filter = 5;
  }

  // Other existing fields...
}

This proposal ensures that all recurrence filter fields are optional, allowing for flexible filtering based on various criteria.

[thomas-nicolai-frequenz]

I don't understand this. If is_recurring is set to true how does one filter by e.g. the end_critea (recurrence details)? One can either provide is_recurring or filter but not both because of the oneof constrain.

[Marenz]

Well, if filter is set it is implicitly true that the dispatch must be recurring, otherwise, none of the provided filters would make sense :) Basically, the recurrence { is_recurring: true } case is only for when you don't care about any specific recurrence filters. As soon as you want to be specific, you use recurrence { filter: .. }

[llucax]

Put another way, if you want to retrieve all dispatches that has a recurrence rule, how can you do that using a RecurrenceFilter but without introducing any further filtering? is_recurring is basically a wildcard.

[thomas-nicolai-frequenz]

I'd prefer you initial suggestion @Marenz

 message RecurrenceFilter {
    // Filter by recurring status
    // True: Only recurring dispatches will be returned.
    // False: Only non-recurring dispatches will be returned.
    // Unset: Both recurring and non-recurring dispatches will be returned, 
    //        recurring dispatches will be filtered based on the recurrence
    //        filter, while non-recurring dispatches will be returned regardless.
    // Examples: 
    // - To retrieve only recurring dispatches:
    //   filter { recurrence_filter { is_recurring: true } }
    // - To retrieve only non-recurring dispatches:
    //   filter { recurrence_filter { is_recurring: false } }
    // - To retrieve all dispatches:
    //   filter { recurrence_filter {} }
    // - To retrieve only recurring dispatches with a specific frequency:
    //   filter { recurrence_filter { is_recurring: true, freq: [FREQUENCY_DAILY] } }
    // - To retrieve only recurring dispatches with a specific frequency but
    //   also include non-recurring dispatches:
    //   filter { recurrence_filter { freq: [FREQUENCY_DAILY] } }
    optional bool is_recurring = 1;

    // Filter by frequency
    // If not specified, all frequencies will be returned.
    repeated RecurrenceRule.Frequency freq = 2;
  }

[Marenz]

that proposal allows the invalid case of reccurence { is_recurring: False, freq: DAILY } and further not-yet added attirbutes though

[llucax]

May I ask again why? There is an issue with that option, it is less type safe or more ambiguous. If someone sense: { is_recurring: false, freq = { ... whatever other filter ... } what does that mean? Which one should we follow?

The oneof completely removes this problem. Saying I don't like oneof is not good enough :laughing:

[thomas-nicolai-frequenz]

May I ask again why

Simply because of my question as no one would understand how to filter by e.g. the end_critea trying to set is_recurring to true. Defaulting something to true if not present even not as a variable is not very intuitive. It feels like a misuse of the intention of oneof.

The oneof completely removes this problem.

What we are trying is to remove any kind of implicit dependency, constraints or ambiguity and move that to the protobuf protocol level. Thats impossible to achieve anyhow. So why are we aiming for this in the first place.

that proposal allows the invalid case of reccurence { is_recurring: False, freq: DAILY } and further not-yet added attirbutes though

Yeah by making it not easier to understand. And this case would happen in any RESTful implementation too as in many other cases. It feels like we are trying to prevent the unpreventable. Its impossible to encode all constraints in protobuf and I would not consider this the common sense approach to it. Just add it to the documentation that this case would be invalid. Just imagine more options that can't be combined. Do we really wanna go down this path of having oneofs depending on other oneofs etc. Where does this stop? If is_recurring is set to false any reasonable person reading the doc or using logical thinking will understand that you can't set this to false and at the same time filter for a daily frequency.

[Marenz]

I believe this confusion is rather minor and is addressed well with the given examples in the definition

What we are trying is to remove any kind of implicit dependency, constraints or ambiguity and move that to the protobuf protocol level. Thats impossible to achieve anyhow. So why are we aiming for this in the first place.

That sounds a bit fatalistic. We can very easily avoid a server-side filter and error check here by using the proposed oneof solution and as far as I understand we want as much as possible straight in the proto.

What the alternative proposal introduces is exactly what you argue we shouldn't do: a case that is unclear/ambigious. With oneof it might not be completely clear at first glance, but it is defined exactly with no room for wrong interpretation.

I am not sure it's easier to understand when the definition is, with the cases luca already showed

If is_recurring is set to false any reasonable person reading the doc or using logical thinking will understand that you can't set this to false and at the same time filter for a daily frequency.

Maybe, but it is now an extra case that needs to be checked and/or denied by the server. Also, I feel exactly the same argument can be said about the oneof solution.

Consider also that our target audience are developers working straight with protobuf definition...

[llucax]

After some internal discussion we found that oneof is actually used extensively in some major APIs, like Google APIs. In particular to express OR for rules and repeated to express AND (or a whole message, of course).

Here are some examples of filtering in Google APIs:

• https://github.com/googleapis/googleapis/blob/3ce84c2ca3f3e453e62bbb9bffb62f3f8ec3f016/google/cloud/billing/budgets/v1/budget_model.proto#L239-L372
• https://github.com/googleapis/googleapis/blob/3ce84c2ca3f3e453e62bbb9bffb62f3f8ec3f016/google/cloud/networkconnectivity/v1/policy_based_routing.proto#L117-L214

Because of this we agreed to move forward with this scheme to express OR and AND in filter rules. There are some examples implementing rules that can combine an arbitrary amount or OR and AND combinations, but I think it is a bit overkill for this stage:

• https://github.com/googleapis/googleapis/blob/3ce84c2ca3f3e453e62bbb9bffb62f3f8ec3f016/google/firestore/v1/query.proto#L41-L243