-
Notifications
You must be signed in to change notification settings - Fork 96
chore: Reactive resource support for debugging connectivity MCP-80 #413
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
Changes from 10 commits
84aa8ba
ce6d2e9
26e1407
c042cff
76ddf15
c5b696b
235ffcf
7dc6746
550a284
e50d706
d0790b5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,10 +13,14 @@ export interface SessionOptions { | |
apiClientSecret?: string; | ||
} | ||
|
||
export class Session extends EventEmitter<{ | ||
export type SessionEvents = { | ||
connected: []; | ||
close: []; | ||
disconnect: []; | ||
}> { | ||
"connection-error": [string]; | ||
}; | ||
|
||
export class Session extends EventEmitter<SessionEvents> { | ||
sessionId?: string; | ||
serviceProvider?: NodeDriverServiceProvider; | ||
apiClient: ApiClient; | ||
|
@@ -102,19 +106,30 @@ export class Session extends EventEmitter<{ | |
connectionString, | ||
defaultAppName: `${packageInfo.mcpServerName} ${packageInfo.version}`, | ||
}); | ||
this.serviceProvider = await NodeDriverServiceProvider.connect(connectionString, { | ||
productDocsLink: "https://github.com/mongodb-js/mongodb-mcp-server/", | ||
productName: "MongoDB MCP", | ||
readConcern: { | ||
level: connectOptions.readConcern, | ||
}, | ||
readPreference: connectOptions.readPreference, | ||
writeConcern: { | ||
w: connectOptions.writeConcern, | ||
}, | ||
timeoutMS: connectOptions.timeoutMS, | ||
proxy: { useEnvironmentVariableProxies: true }, | ||
applyProxyToOIDC: true, | ||
}); | ||
|
||
try { | ||
this.serviceProvider = await NodeDriverServiceProvider.connect(connectionString, { | ||
productDocsLink: "https://github.com/mongodb-js/mongodb-mcp-server/", | ||
productName: "MongoDB MCP", | ||
readConcern: { | ||
level: connectOptions.readConcern, | ||
}, | ||
readPreference: connectOptions.readPreference, | ||
writeConcern: { | ||
w: connectOptions.writeConcern, | ||
}, | ||
timeoutMS: connectOptions.timeoutMS, | ||
proxy: { useEnvironmentVariableProxies: true }, | ||
applyProxyToOIDC: true, | ||
}); | ||
|
||
await this.serviceProvider?.runCommand?.("admin", { hello: 1 }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NodeDriverServiceProvider.connect, despite it's name, does not in fact connect. This behaviour is likely inherited from the driver. By forcing a ping (by using hello) we force a connection and validate that it's in fact correct. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we have a ping on atlas cluster connect tool where we added a ping, should we drop it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see that it's used for answering the client with the connection status. I think we can keep it for now and then when we remove this logic to the new ConnectionManager (Tech Design) we can refactor and use the same code for both connections. How do you feel about this strategy? |
||
} catch (error: unknown) { | ||
const message = error instanceof Error ? error.message : `${error as string}`; | ||
this.emit("connection-error", message); | ||
throw error; | ||
} | ||
|
||
this.emit("connected"); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
import { ReactiveResource } from "../resource.js"; | ||
import { config } from "../../common/config.js"; | ||
import type { UserConfig } from "../../common/config.js"; | ||
|
||
export class ConfigResource extends ReactiveResource( | ||
{ | ||
name: "config", | ||
uri: "config://config", | ||
config: { | ||
description: | ||
"Server configuration, supplied by the user either as environment variables or as startup arguments", | ||
}, | ||
}, | ||
{ | ||
initial: { ...config }, | ||
events: [], | ||
} | ||
) { | ||
reduce(eventName: undefined, event: undefined): UserConfig { | ||
void eventName; | ||
void event; | ||
Comment on lines
+19
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could possibly be avoided by having reduce without any params? |
||
|
||
return this.current; | ||
} | ||
|
||
toOutput(): string { | ||
const result = { | ||
telemetry: this.current.telemetry, | ||
logPath: this.current.logPath, | ||
connectionString: this.current.connectionString | ||
? "set; access to MongoDB tools are currently available to use" | ||
: "not set; before using any MongoDB tool, you need to configure a connection string, alternatively you can setup MongoDB Atlas access, more info at 'https://github.com/mongodb-js/mongodb-mcp-server'.", | ||
connectOptions: this.current.connectOptions, | ||
atlas: | ||
this.current.apiClientId && this.current.apiClientSecret | ||
? "set; MongoDB Atlas tools are currently available to use" | ||
: "not set; MongoDB Atlas tools are currently unavailable, to have access to MongoDB Atlas tools like creating clusters or connecting to clusters make sure to setup credentials, more info at 'https://github.com/mongodb-js/mongodb-mcp-server'.", | ||
}; | ||
|
||
return JSON.stringify(result); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
import { ReactiveResource } from "../resource.js"; | ||
|
||
type ConnectionStateDebuggingInformation = { | ||
readonly tag: "connected" | "connecting" | "disconnected" | "errored"; | ||
readonly connectionStringAuthType?: "scram" | "ldap" | "kerberos" | "oidc-auth-flow" | "oidc-device-flow" | "x.509"; | ||
readonly oidcLoginUrl?: string; | ||
readonly oidcUserCode?: string; | ||
readonly errorReason?: string; | ||
}; | ||
|
||
export class DebugResource extends ReactiveResource( | ||
{ | ||
name: "debug-mongodb-connectivity", | ||
uri: "debug://mongodb-connectivity", | ||
config: { | ||
description: "Debugging information for connectivity issues.", | ||
}, | ||
}, | ||
{ | ||
initial: { tag: "disconnected" } as ConnectionStateDebuggingInformation, | ||
events: ["connected", "disconnect", "close", "connection-error"], | ||
} | ||
) { | ||
reduce( | ||
eventName: "connected" | "disconnect" | "close" | "connection-error", | ||
event: string | undefined | ||
): ConnectionStateDebuggingInformation { | ||
void event; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this necessary anymore, now that you're using |
||
|
||
switch (eventName) { | ||
case "connected": | ||
return { tag: "connected" }; | ||
case "connection-error": | ||
return { tag: "errored", errorReason: event }; | ||
case "disconnect": | ||
case "close": | ||
return { tag: "disconnected" }; | ||
} | ||
} | ||
|
||
toOutput(): string { | ||
let result = ""; | ||
|
||
switch (this.current.tag) { | ||
case "connected": | ||
result += "The user is connected to the MongoDB cluster."; | ||
break; | ||
case "errored": | ||
result += `The user is not connected to a MongoDB cluster because of an error.\n`; | ||
result += `<error>${this.current.errorReason}</error>`; | ||
break; | ||
case "connecting": | ||
case "disconnected": | ||
result += "The user is not connected to a MongoDB cluster."; | ||
break; | ||
} | ||
|
||
return result; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
import { Server } from "../server.js"; | ||
import { Session } from "../common/session.js"; | ||
import { UserConfig } from "../common/config.js"; | ||
import { Telemetry } from "../telemetry/telemetry.js"; | ||
import type { SessionEvents } from "../common/session.js"; | ||
import { ReadResourceCallback, ResourceMetadata } from "@modelcontextprotocol/sdk/server/mcp.js"; | ||
import logger, { LogId } from "../common/logger.js"; | ||
|
||
type PayloadOf<K extends keyof SessionEvents> = SessionEvents[K][0]; | ||
|
||
type ResourceConfiguration = { name: string; uri: string; config: ResourceMetadata }; | ||
|
||
export function ReactiveResource<Value, RelevantEvents extends readonly (keyof SessionEvents)[]>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [q] any reason to use the function returning class pattern instead of using a class straight away?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My understanding is that resource metadata is immutable and by using this pattern I feel it's more convenient to specify the configuration once and still be type-safe. I think using normal classes where everything is specified in properties makes more sense when you want to change the state. With this pattern, it's not possible, as the metadata is hidden in the closure. |
||
{ name, uri, config: resourceConfig }: ResourceConfiguration, | ||
{ | ||
initial, | ||
events, | ||
}: { | ||
initial: Value; | ||
events: RelevantEvents; | ||
} | ||
) { | ||
type SomeEvent = RelevantEvents[number]; | ||
|
||
abstract class NewReactiveResource { | ||
protected readonly session: Session; | ||
protected readonly config: UserConfig; | ||
protected current: Value; | ||
|
||
constructor( | ||
protected readonly server: Server, | ||
protected readonly telemetry: Telemetry, | ||
current?: Value | ||
) { | ||
this.current = current ?? initial; | ||
this.session = server.session; | ||
this.config = server.userConfig; | ||
|
||
for (const event of events) { | ||
this.session.on(event, (...args: SessionEvents[typeof event]) => { | ||
this.reduceApply(event, (args as unknown[])[0] as PayloadOf<typeof event>); | ||
void this.triggerUpdate(); | ||
}); | ||
} | ||
} | ||
|
||
public register(): void { | ||
this.server.mcpServer.registerResource(name, uri, resourceConfig, this.resourceCallback); | ||
} | ||
|
||
private resourceCallback: ReadResourceCallback = (uri) => ({ | ||
contents: [ | ||
{ | ||
text: this.toOutput(), | ||
mimeType: "application/json", | ||
uri: uri.href, | ||
}, | ||
], | ||
}); | ||
|
||
private async triggerUpdate() { | ||
try { | ||
await this.server.mcpServer.server.sendResourceUpdated({ uri }); | ||
this.server.mcpServer.sendResourceListChanged(); | ||
} catch (error: unknown) { | ||
logger.warning( | ||
LogId.serverClosed, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This most likely needs a new log id? |
||
"Could not send the latest resources to the client.", | ||
error as string | ||
); | ||
} | ||
} | ||
|
||
reduceApply(eventName: SomeEvent, ...event: PayloadOf<SomeEvent>[]): void { | ||
this.current = this.reduce(eventName, ...event); | ||
} | ||
|
||
protected abstract reduce(eventName: SomeEvent, ...event: PayloadOf<SomeEvent>[]): Value; | ||
abstract toOutput(): string; | ||
} | ||
|
||
return NewReactiveResource; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
import { ConfigResource } from "./common/config.js"; | ||
import { DebugResource } from "./common/debug.js"; | ||
|
||
export const Resources = [ConfigResource, DebugResource] as const; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
import { beforeEach, describe, expect, it } from "vitest"; | ||
import { DebugResource } from "../../../../src/resources/common/debug.js"; | ||
import { Session } from "../../../../src/common/session.js"; | ||
import { Server } from "../../../../src/server.js"; | ||
import { Telemetry } from "../../../../src/telemetry/telemetry.js"; | ||
import { config } from "../../../../src/common/config.js"; | ||
|
||
describe("debug resource", () => { | ||
// eslint-disable-next-line | ||
const session = new Session({} as any); | ||
// eslint-disable-next-line | ||
const server = new Server({ session } as any); | ||
const telemetry = Telemetry.create(session, { ...config, telemetry: "disabled" }); | ||
|
||
let debugResource: DebugResource = new DebugResource(server, telemetry); | ||
|
||
beforeEach(() => { | ||
debugResource = new DebugResource(server, telemetry); | ||
}); | ||
|
||
it("should be connected when a connected event happens", () => { | ||
debugResource.reduceApply("connected", undefined); | ||
const output = debugResource.toOutput(); | ||
|
||
expect(output).toContain(`The user is connected to the MongoDB cluster.`); | ||
}); | ||
|
||
it("should be disconnected when a disconnect event happens", () => { | ||
debugResource.reduceApply("disconnect", undefined); | ||
const output = debugResource.toOutput(); | ||
|
||
expect(output).toContain(`The user is not connected to a MongoDB cluster.`); | ||
}); | ||
|
||
it("should be disconnected when a close event happens", () => { | ||
debugResource.reduceApply("close", undefined); | ||
const output = debugResource.toOutput(); | ||
|
||
expect(output).toContain(`The user is not connected to a MongoDB cluster.`); | ||
}); | ||
|
||
it("should be disconnected and contain an error when an error event occurred", () => { | ||
debugResource.reduceApply("connection-error", "Error message from the server"); | ||
const output = debugResource.toOutput(); | ||
|
||
expect(output).toContain(`The user is not connected to a MongoDB cluster because of an error.`); | ||
expect(output).toContain(`<error>Error message from the server</error>`); | ||
}); | ||
}); |
Uh oh!
There was an error while loading. Please reload this page.