Allow sending signals to specific clients #17729
Conversation
github-actions
bot
added
area: driver
github-actions
bot
removed
the
area: server
⯅ @fluid-example/bundle-size-tests: +483 Bytes
Baseline commit: 7c8cdb8 |
| public submitSignal(message: IDocumentMessage): void { | ||
| this.emitMessages("submitSignal", [[message]]); | ||
| public submitSignal(content: unknown, targetClientId?: string): void { | ||
| const signal: ISentSignalMessage = { |
There was a problem hiding this comment.
where does this get unwrapped? does the server do it? i worry the change is shape won't be handles by clients. over all this needs tests
There was a problem hiding this comment.
The server will process the ISentSignalMessage's and send the signal to the specific client when specified.
The signals received by clients will look identical to how they are today - there is no breaking change / requirements for clients to all be on a new version.
| }; | ||
|
|
||
| // back-compat: the typing for this method and emitMessages is incorrect, will be fixed in a future PR | ||
| this.emitMessages("submitSignal", [signal] as any); |
There was a problem hiding this comment.
should this be [[signal]], as that is what was done before
There was a problem hiding this comment.
I currently have PUSH setup to accept a single batch of signals per submitSignal emit when the client is passing submit_signals_v2 in supportedFeature. The server typing looks like sentSignalMessages: ISentSignalMessage[] so this is correct.
It did not seem like nesting a single signal into 2 arrays was ever used / necessary, especially given that existing clients cannot currently be sent a batch of signals - the server always needs to split them up anyway.
Co-authored-by: Tony Murphy <anthony.murphy@microsoft.com>
| @@ -296,8 +298,11 @@ export class OdspDocumentDeltaConnection extends DocumentDeltaConnection { | |||
| relayUserAgent: [client.details.environment, ` driverVersion:${pkgVersion}`].join(";"), | |||
| }; | |||
|
|
|||
| connectMessage.supportedFeatures = { | |||
| [feature_submit_signals_v2]: true, | |||
| }; | |||
There was a problem hiding this comment.
Is there a value for client to convey this info? Just curions
There was a problem hiding this comment.
Yes. Push will handle the submit_signal event differently when this feature is passed. It changes the structure of the incoming message and adds support for the targetClientId property
| }; | ||
|
|
||
| // back-compat: the typing for this method and emitMessages is incorrect, will be fixed in a future PR | ||
| this.emitMessages("submitSignal", [signal] as any); |
There was a problem hiding this comment.
This change assumes PUSH can handle (auto-detect) old and new format, right?
It used to be IDocumentMessage[][], now it's {content: IDocumentMessage, targetClientId?: string}[]
I'm not sure why it was same shape as ops (essentially indicating support for some kind of batching).
There was a problem hiding this comment.
IDocumentMessage[][] has never been the correct typing. content: IDocumentMessage is also wrong.
It should be content: unknown but I'm not going to introduce a breaking typing change in this PR to resolve this
…rror is not supported
Yes the ODSP side of this is ready. |
## Description Follow up to #17729 There was a missing change in this PR to add targetClientId support for signals. Confirmed with the PR author that this was an accidental omission.
…9475) ## Description Follow up to microsoft#17729 There was a missing change in this PR to add targetClientId support for signals. Confirmed with the PR author that this was an accidental omission.
## Description Updating ISignalMessage to inlcude optional targetSignalid. This can be useful for receiving clients trying to understand the overall approximate load on service and maybe other understanding. If received signal has that property, then we know it wasn't broadcast to everyone. ### FF API Council Context I have a minor API change related to ISignalMessage. [ODSP had made changes to allow for targeted signals](#17729) and I have been tasked with doing some of the follow-up work related to this. Initially the idea was for the server to strip the targetClientId before sending the signal message to the targeted client, however Jason and Gary came to an agreement that it would be beneficial to leave this information on the message, which would involve adding an optional targetClientId member to ISignalMessage(Base). This could be useful for receiving clients trying to understand the overall approximate load on service and maybe other understanding. If received signal has that property, then we know it wasn't broadcast to everyone. Thanks.
Description
This PR adds the ability for clients to send signals to specific clients instead of always broadcasting signals to everyone. This can be used by Loop applications to drastically reduce the number of signal packets being sent in client join scenarios, which will reduce server resource utilization.
Brief rationale behind this change:
Current system:
50 * 51 = 2550Future system that leverages this change:
50So this change would result in a large reduction in the number of packets being sent out for that case.
Breaking Changes
targetClientId?: stringargument tosubmitSignal/submitDataStoreSignalReviewer Guidance
IConnect.supportedFeatures