-
Notifications
You must be signed in to change notification settings - Fork 339
Pass session id's to MCP endpoints. #466
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
base: main
Are you sure you want to change the base?
Pass session id's to MCP endpoints. #466
Conversation
/// </summary> | ||
public InitializeRequestParams? InitializeRequest { get; internal set; } | ||
public Action<InitializeRequestParams>? OnInitRequestReceived { get; set; } |
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.
It feels like this could be generalized (async callback? passing additional context?) but for now I went for the simplest solution possible for the task at hand.
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.
@halter73 I converted this to a Func<InitializeRequestParams, ValueTask>
for the sake of minimizing the probability of future amendments.
@@ -52,6 +52,15 @@ public async Task Connect_TestServer_ShouldProvideServerFields() | |||
// Assert | |||
Assert.NotNull(client.ServerCapabilities); | |||
Assert.NotNull(client.ServerInfo); | |||
|
|||
if (ClientTransportOptions.Endpoint.AbsolutePath.EndsWith("/sse")) |
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.
That's the best way I could come up for determining if the transport ends up using legacy sse.
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.
That seems good enough for the purpose of this test. You could also check the TransportMode and read the client logs to determine whether or not it fell back in AutoDetect mode, but this seems easier.
Do you think we should add a client API to detect which transport was actually used in auto mode? I'm not sure how useful it'd be outside of logging.
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.
Do you think we should add a client API to detect which transport was actually used in auto mode? I'm not sure how useful it'd be outside of logging.
Why not? Seems useful from a debugging and diagnostics perspective I would think.
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.
Other than exposing the SessionId, is there any other reason we want to expose the ITransport on the IMcpEndpoint? If not, should we start with exposing just the SessionId and not the entire ITransport?
I'd also consider just telling people to use IHttpContextAccessor to read the mcp-session-id response header inside their tools and other handlers if they care. It should be just as reliable without mucking up our core interfaces like ITransport and IMcpEndpoint with stuff that isn't that relevant to non-HTTP transports. This is more discoverable though for people creating their own logs, so I see the appeal there, but maybe we could just add documentation and hold off on the API change.
I think this would be more valuable if we included the session id in all the logs that can realistically have them similar to what we do for Kestrel with connection IDs and request IDs.
@@ -52,6 +52,15 @@ public async Task Connect_TestServer_ShouldProvideServerFields() | |||
// Assert | |||
Assert.NotNull(client.ServerCapabilities); | |||
Assert.NotNull(client.ServerInfo); | |||
|
|||
if (ClientTransportOptions.Endpoint.AbsolutePath.EndsWith("/sse")) |
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.
That seems good enough for the purpose of this test. You could also check the TransportMode and read the client logs to determine whether or not it fell back in AutoDetect mode, but this seems easier.
Do you think we should add a client API to detect which transport was actually used in auto mode? I'm not sure how useful it'd be outside of logging.
Exposing the transport seems like the reasonable thing to do given that each MCP endpoint corresponds to one instance. What are some of the drawbacks of doing it? Lack of encapsulation? I have no strong opinions on the matter.
Sure, but this only works provided that you only use your tools with streamable http exclusively. If your tool is invoked via legacy sse there is no |
Co-authored-by: Stephen Halter <[email protected]>
Co-authored-by: Stephen Halter <[email protected]>
35fd9a3
to
ff9421a
Compare
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.
What are some of the drawbacks of doing it? Lack of encapsulation? I have no strong opinions on the matter.
Mostly the lack of encapsulation. We can always update this later, but I'd prefer to just expose the string? SessionId on the IMcpEndpoint for now.
Ensures that MCP servers and clients have access to the underlying session id (typically the value of the
Mcp-Session-Id
header in streamable http). It does so by making the following (breaking) API changes:SessionId
property to theITransport
interface.Transport
property to theIMcpEndpoint
interface.This lets consumers of
IMcpServer
andIMcpClient
obtain the underlying session id by evaluatingendpoint.Transport.SessionId
. Note that the session id can be null if