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

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

Open
tiyash-basu-frequenz opened this issue Mar 22, 2024 · 16 comments
Assignees
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

@tiyash-basu-frequenz
Copy link
Contributor

tiyash-basu-frequenz commented Mar 22, 2024

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

  • Copy-pasted from the issue linked below:
    "This could be used to indicate the last activity or last command that was issued to an API, e.g., a charge command to the Dispatch API or Microgrid API. (such tags could be set on these APIs through their own RPCs)."
  • There are some cases where the AC electricity data is not enough to identify what's going on. E.g., in case of the FCR app, this could help determine operating-point-offset (assuming the app uses it properly). In this example, obtaining the operating-point-offset just from AC data would be difficult, if not impossible.
  • More broadly, this could also be used to stream arbitrary data through the Microgrid API/Reporting API. E.g., ATM, there is no metric that captures the FCR operating point. This feature would open up the API to send arbitrary information, so the FCR app can stream the operating point.

Alternatives and workarounds

No response

Additional context

frequenz-floss/frequenz-api-common#207

@tiyash-basu-frequenz tiyash-basu-frequenz 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 Mar 22, 2024
@tiyash-basu-frequenz tiyash-basu-frequenz added this to the v0.17.0 milestone Mar 22, 2024
@llucax
Copy link
Contributor

llucax commented Mar 28, 2024

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
Copy link
Contributor Author

tiyash-basu-frequenz commented Mar 28, 2024

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
Copy link
Contributor

llucax commented Mar 28, 2024

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
Copy link
Contributor Author

tiyash-basu-frequenz commented Mar 28, 2024

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
Copy link
Contributor

llucax commented Mar 28, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

Marenz commented Apr 8, 2024

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
Copy link
Contributor Author

tiyash-basu-frequenz commented Apr 8, 2024

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.,

  • users could update the tag after issuing charge/discharge commands. That would tell them whether the data they receive is a result of their command or not.
  • this could also be used to stream arbitrary data through the Microgrid API/Reporting API. I think about the working point from the FCR actor. ATM, that there is no metric that captures it. Something like this would open up the API to send arbitrary tags, so that users can publish any data they want, e.g., the working point.

@llucax
Copy link
Contributor

llucax commented Apr 8, 2024

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
Copy link
Contributor Author

sure

@Marenz
Copy link
Contributor

Marenz commented Apr 8, 2024

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

@tiyash-basu-frequenz
Copy link
Contributor Author

tiyash-basu-frequenz commented Apr 8, 2024

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: #223 (comment).

@tiyash-basu-frequenz
Copy link
Contributor Author

tiyash-basu-frequenz commented Apr 8, 2024

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 tiyash-basu-frequenz self-assigned this Apr 9, 2024
@tiyash-basu-frequenz
Copy link
Contributor Author

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

  • 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.
  • This is not a high priority issue. I will move it to the backlog for now.

@tiyash-basu-frequenz tiyash-basu-frequenz modified the milestones: v0.17.0, Backlog Apr 12, 2024
@thomas-nicolai-frequenz
Copy link

thomas-nicolai-frequenz commented Apr 22, 2024

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.

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

4 participants