frequenz-floss / frequenz-api-microgrid

gRPC+protobuf specification and Python bindings for the Frequenz Microgrid API
https://frequenz-floss.github.io/frequenz-api-microgrid/
MIT License
6 stars 6 forks source link

Add RPC to specify an additional payload to stream alonwith component data #223

Open tiyash-basu-frequenz opened 6 months ago

tiyash-basu-frequenz commented 6 months ago

What's needed?

We need an RPC to specify an additional payload to stream along with component data. This payload would be overwritten by the next call to this RPC.

Proposed solution

One solution would be to add the following RPC with the specified request message:

service Microgrid {
  ...
  rpc AddAdditionalPayload(AddAdditionalPayloadRequest) returns (google.protobuf.Empty)
}
message AddAdditionalPayloadRequest {
    google.protobuf.Struct payload = 1;
}

The service would attach this payload to the outgoing data message according to the specs defined in frequenz-api-common.

Use cases

Alternatives and workarounds

No response

Additional context

https://github.com/frequenz-floss/frequenz-api-common/issues/207

llucax commented 6 months ago

Maybe it would be worth adding something to the use case section of the issue, at least for me it is not clear what is the intended use of this feature.

tiyash-basu-frequenz commented 6 months ago

Maybe it would be worth adding something to the use case section of the issue, at least for me it is not clear what is the intended use of this feature.

Already mentioned: "We need an RPC to specify an additional payload to stream along with component data."

But please feel free to update the UseCase section.

llucax commented 6 months ago

For me that describe what the feature does, but not why it is needed. Why do we need specify an additional payload to stream along with component data? Who's going to use it and for what?

tiyash-basu-frequenz commented 6 months ago

This was mentioned in the linked issue. I copy pasted the text from there. (While we are discussing this, I will also mention that the issue template feels odd to me. The Use-Case should be present before the Solution szection. I generally add the "why" in the "what's needed" section, because mentioning it after proposing a solution feels odd.)

llucax commented 6 months ago

This was mentioned in the linked issue. I copy pasted the text from there.

OK, I read the linked issue too but I still find it difficult to understand or see the use case, but maybe it is me just lacking some additional context.

(While we are discussing this, I will also mention that the issue template feels odd to me. The Use-Case should be present before the Solution szection. I generally add the "why" in the "what's needed" section, because mentioning it after proposing a solution feels odd.)

I'm not surprised, I created the template a long time ago as something experimental and wasn't improved since then :D.

That said, I think moving the use case before the proposed solution might be a bit noisy, at least in my experience use case is empty in 90% (?) of the cases, so it will just be distracting. I suggested GitHub to add an option to skip empty sections, which really don't make sense to put in the issue at all, but I doubt it will be implemented soon (or at all), so we could explore with different template approaches to avoid the noise.

tiyash-basu-frequenz commented 6 months ago

OK, I read the linked issue too but I still find it difficult to understand or see the use case, but maybe it is me just lacking some additional context.

Okay, fair enough, I will try to extend the issue with more context.

so we could explore with different template approaches to avoid the noise.

Sounds good.

tiyash-basu-frequenz commented 6 months ago

I will try to extend the issue with more context.

I added some additional text to the use-case section here and in the linked issue frequenz-floss/frequenz-api-common#207.

Marenz commented 6 months ago

To me that issues is still confusing. Who would be the sender and who the receiver of that data? How often would it be sent?

tiyash-basu-frequenz commented 6 months ago

Who would be the sender and who the receiver of that data?

Users, with their SDK apps. This data will also be available via the Reporting API.

How often would it be sent?

As frequently as they want.

To me that issues is still confusing.

Think of it as a user-specified tag. E.g.,

llucax commented 6 months ago

OK, now that I'm not the only one that doesn't understand this feature, I will keep pushing a little bit further, as for me it is still unclear how this is supposed to work exactly and what are real use ceases. For me it would be good to have some pseudo code but real example, like, I don't know, for FCR:

working_point = calculate_working_point()
client.charge(battery_id, tags={"working_point": working_point})
# now what?
async data for client.get_data(battery_id, "soc"):
     # data here will have somehow the tags attached here?
     # How is FCR supposed to use this information?

Maybe we should have a meeting to discuss/explain this feature better before moving forward?

tiyash-basu-frequenz commented 6 months ago

sure

Marenz commented 6 months ago

My friends, please, it's operating point, not working point. :)

tiyash-basu-frequenz commented 6 months ago

I got this feedback from @Marenz in a call:

Why stream the data back with the component data stream? Why not have a separate RPC for it?

Edit: The idea is to annotate a data sample. Therefore, it would be easier if this were to be present in the component data stream. For reference: https://github.com/frequenz-floss/frequenz-api-microgrid/issues/223#issuecomment-2051394531.

tiyash-basu-frequenz commented 6 months ago

Feedback from @llucax:

Why have this feature in the Microgrid API at all? Just for convenience?

Edit: Yes, otherwise it would be a lot of additional work to associate tags with data samples. Maybe we need to give more though to this once this issue gets prioritised.

tiyash-basu-frequenz commented 5 months ago

I got a few inputs from @thomas-nicolai-frequenz :

thomas-nicolai-frequenz commented 5 months ago

The tag, when being returned in the data stream, will only be present in one sample. This resembles annotating a data sample in timeseries data.

Not sure thats what I exactly said. What I have in mind is that we attach the payload to a timestamp not sample. In doubt it would be attached to the next timestamp that we have samples for. I would also like to refrain from using the term tag. That means something else. Payload is a json struct that can contain all sorts of data. It should also be said that it might or not might include custom metrics but thats something to decide if we'd want to handle this separately.

P.S. That all said we could of course also allow adding payloads to samples but thats a different type of feature and not what I had originally in mind.