-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(protocol): Debounce notifications to improve network efficiancy #746
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
Conversation
I came here to open an issue about this. Thank you for the pull request! |
Does this PR also cover not sending a list change notification if things did not actually change? Because right now I'm having to do some enabled checking before I enable or disable a tool. Here's my code right now: async function updateTools() {
const entries = await agent.db.getEntries()
if (entries.length > 0) {
if (!deleteEntryTool.enabled) deleteEntryTool.enable()
if (!updateEntryTool.enabled) updateEntryTool.enable()
if (!listEntriesTool.enabled) listEntriesTool.enable()
if (!getEntryTool.enabled) getEntryTool.enable()
} else {
if (deleteEntryTool.enabled) deleteEntryTool.disable()
if (updateEntryTool.enabled) updateEntryTool.disable()
if (listEntriesTool.enabled) listEntriesTool.disable()
if (getEntryTool.enabled) getEntryTool.disable()
}
const tags = await agent.db.getTags()
if (tags.length > 0) {
if (!deleteTagTool.enabled) deleteTagTool.enable()
if (!updateTagTool.enabled) updateTagTool.enable()
if (!listTagsTool.enabled) listTagsTool.enable()
if (!getTagTool.enabled) getTagTool.enable()
} else {
if (deleteTagTool.enabled) deleteTagTool.disable()
if (updateTagTool.enabled) updateTagTool.disable()
if (listTagsTool.enabled) listTagsTool.disable()
if (getTagTool.enabled) getTagTool.disable()
}
if (entries.length > 0 && tags.length > 0) {
if (!addTagToEntryTool.enabled) addTagToEntryTool.enable()
} else {
if (addTagToEntryTool.enabled) addTagToEntryTool.disable()
}
}
agent.db.subscribe(updateTools)
await updateTools() I would love to not have to worry about checking whether something's enabled already. |
Hi @kentcdodds, np for the PR! It does not currently perform a noop for tools as you suggest, though I'd love that functionality as well. I'm currently doing the same thing as you, with the conditional checks. I'd be down to open a PR to change the current functionality of throwing an error to a noop if that's the general consensus. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for implementing this!
A few things to consider:
- Silent Data Loss - When multiple notifications with different parameters are sent, only the FIRST one's params are kept. We either need to be absolutely clear that only notification without any data should be debounces in readme or add checks in the code (I'd probably prefer this)
- same for relatedRequestId
- clean up on connection close
Thanks for the review @ihrpr. I've gone ahead and updated the PR to address the bullet points you mentioned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you
This PR introduces a micro-batching mechanism for notifications within the protocol layer to improve network efficiency by coalescing rapid, successive notifications into a single message.
Motivation and Context
Currently, SDK methods that trigger notifications (e.g.,
tool.enable()
) do so immediately. When an application performs bulk operations, such as enabling and disabling multiple tools during a state transition, this results in a "flood" of individual notifications being sent to the client.For instance, consider the following state management code from a benchmark application:
Without this change, the
enter()
method would trigger five separatenotifications/tools/list_changed
messages to be sent to the client, one for each call to.enable()
or.disable()
.This PR solves this inefficiency by debouncing these notifications. With this change, the entire block of five calls results in only a single, coalesced
notifications/tools/list_changed
message, significantly reducing network traffic and server load.How Has This Been Tested?
This feature has been validated with a new suite of unit tests and verified in the real-world application that uses the state machine shown above.
Unit Tests:
send
.setImmediate
orPromise.resolve().then()
) to reliably test the asynchronous debouncing logic.Application Testing:
IdleState
class.IdleState.enter()
now result in a single, consolidatedtools/list_changed
notification, as expected.Breaking Changes
No. This is a non-breaking, opt-in feature. Existing code will continue to function without any changes. To enable the new behavior, users must explicitly provide the new
debouncedNotificationMethods
configuration option.Types of changes
Checklist
Additional context
The implementation uses a microtask-based debouncing strategy within the
Protocol.notification()
method. When a notification designated for debouncing is called, it schedules the actual network send for the next microtask usingPromise.resolve().then()
. This allows all other synchronous calls within the same event-loop tick to be captured and coalesced into that single scheduled send.This design was chosen specifically to avoid altering the public-facing SDK API. No new methods like
server.batchedUpdates()
are required, making the feature transparent to the end-user's application code once configured.