Skip to content

Conversation

@llucax
Copy link
Contributor

@llucax llucax commented Oct 29, 2024

No description provided.

@llucax llucax requested a review from a team as a code owner October 29, 2024 09:46
@llucax llucax requested review from phillip-wenig-frequenz and removed request for a team October 29, 2024 09:46
@github-actions github-actions bot added part:docs Affects the documentation part:microgrid Affects the interactions with the microgrid labels Oct 29, 2024
@llucax llucax self-assigned this Oct 29, 2024
@llucax llucax requested a review from shsms October 29, 2024 09:47
@llucax llucax added this to the v1.0.0-rc1100 milestone Oct 29, 2024
@llucax llucax enabled auto-merge October 29, 2024 09:48
Comment on lines 21 to 23
The channel to use to receive the data stream is determined by the by the client
creating the request, but need to be unique in the channel's registry. For this
a `namespace` is used to make the channel name unique.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't feel like it is from the perspective of the ComponentMetricRequest type.

I came up with, the following, might need shortening/cleanup:

      Along with specifying the component ID and the metric that needs
      to be streamed, `ComponentMetricRequest`s are also used to
      determine the channel used to stream the data.

      Data for all requests with the same namespace, component_id and
      metric_id will be streamed through the same channel.  Normally,
      the actors sending the data would make sure the data is not
      duplicated.

      But when multiple unique channels are required, for example, to
      differentiate between raw and resampled data, the requests can
      specify different `namespace`s in the requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran my text, expanded to include yours, through o1-preview and iterated a couple of times to include all the details I found important. Here is the result

A request to start streaming a component's metric.

Requesters use this class to specify which component's metric they want to subscribe to,
including the component ID, metric ID, and an optional start time. The `namespace` is
defined by the requester and influences the construction of the channel name via the
`get_channel_name()` method.

The `namespace` allows differentiation of data streams for the same component and metric.
For example, requesters can use different `namespace` values to subscribe to raw or resampled
data streams separately. This ensures that each requester receives the appropriate type of
data without interference. Requests with the same `namespace`, `component_id`, and
`metric_id` will use the same channel, preventing unnecessary duplication of data streams.

The channel name must be shared between the requester and provider because both need to
independently retrieve the same channel from the `ChannelRegistry`. By specifying the
`namespace`, requesters determine the channel name, ensuring they can access the correct
channel after subscribing.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels misleading because it presents part of the story as the whole story: "By specifying the namespace, requesters determine the channel name".

I would rephrase the last paragraph as follows:

The requester and provider must use the same channel name so that they can independently retrieve the same channel from the ChannelRegistry. This is achieved by using the get_channel_name method to generate the name on both sides based on parameters set by the requesters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's missing that namespace is key for that unique channel name negotiation. Without it, you can't get a unique channel name for the same metric and component. I'm confused because it is exactly the part you want to remove, but I think it is key to understanding why namespace is there.

Copy link
Contributor

@shsms shsms Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first two paragraphs already explain it very clearly. The third paragraph only has a misleading mention of namespace. I think it makes more sense to focus on the other aspect of how the matching names are achieved on both sides instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, for me it wasn't that clear, but reading it again, I guess it is, I will update with your last paragraph proposal.

@llucax
Copy link
Contributor Author

llucax commented Oct 30, 2024

Updated.

@llucax llucax force-pushed the namespace-docs branch 2 times, most recently from 1b24957 to f6c32ed Compare October 30, 2024 10:13
@llucax llucax requested a review from shsms October 30, 2024 10:35
In particular clarify the use of the `namespace` attribute and its
relationship with the channel name.

Signed-off-by: Leandro Lucarella <[email protected]>
The `start_time` is optional, and rarely used (for the time being not
even supported), so there is no need to include it in the channel name
when it is not present at all.

Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Copy link
Contributor Author

llucax commented Nov 7, 2024

Rebased.

@llucax llucax added this pull request to the merge queue Nov 7, 2024
Merged via the queue into frequenz-floss:v1.x.x with commit 998ebff Nov 7, 2024
18 checks passed
@llucax llucax deleted the namespace-docs branch November 7, 2024 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:docs Affects the documentation part:microgrid Affects the interactions with the microgrid

Projects

Development

Successfully merging this pull request may close these issues.

2 participants