Copilot commented on code in PR #1010:
URL:
https://github.com/apache/skywalking-banyandb/pull/1010#discussion_r2958716134
##########
mcp/src/index.ts:
##########
@@ -33,11 +38,82 @@ dotenv.config();
setupGlobalErrorHandlers();
const BANYANDB_ADDRESS = process.env.BANYANDB_ADDRESS || 'localhost:17900';
-const LLM_API_KEY = process.env.LLM_API_KEY;
-const LLM_BASE_URL = process.env.LLM_BASE_URL;
+const TRANSPORT = process.env.TRANSPORT || 'stdio';
+const MCP_PORT = parseInt(process.env.MCP_PORT || '3000', 10);
+
Review Comment:
`MCP_PORT` is parsed with `parseInt(...)` but not validated. If `MCP_PORT`
is unset/empty/non-numeric, this becomes `NaN` and `httpServer.listen(NaN)`
will throw at runtime. Consider validating `Number.isFinite(MCP_PORT)` (and >0)
and either defaulting or failing fast with a clear error message.
##########
mcp/src/index.ts:
##########
@@ -33,11 +38,82 @@ dotenv.config();
setupGlobalErrorHandlers();
const BANYANDB_ADDRESS = process.env.BANYANDB_ADDRESS || 'localhost:17900';
-const LLM_API_KEY = process.env.LLM_API_KEY;
-const LLM_BASE_URL = process.env.LLM_BASE_URL;
+const TRANSPORT = process.env.TRANSPORT || 'stdio';
+const MCP_PORT = parseInt(process.env.MCP_PORT || '3000', 10);
Review Comment:
PR description mentions `streamable-http` as a transport option, but the
implementation/docs key off `TRANSPORT === 'http'`. To avoid confusion and
config mistakes, either accept both values (e.g., `http` and `streamable-http`)
or align the documentation/PR description and code to one canonical value.
##########
mcp/src/index.ts:
##########
@@ -359,15 +367,67 @@ async function main() {
}
}
+ if (name === 'get_generate_bydbql_prompt') {
+ const queryHints = normalizeQueryHints(args);
+ if (!queryHints.description) {
+ throw new Error('description is required');
+ }
+
+ const { groups, resourcesByGroup } = await
loadQueryContext(banyandbClient);
+ const prompt = generateBydbQL(queryHints.description, queryHints,
groups, resourcesByGroup);
+
+ return { content: [{ type: 'text', text: prompt }] };
+ }
+
throw new Error(`Unknown tool: ${name}`);
});
- // Start the server
- const transport = new StdioServerTransport();
- await server.connect(transport);
+ return server;
+}
+
+async function main() {
+ const banyandbClient = new BanyanDBClient(BANYANDB_ADDRESS);
+
+ log.info('Using built-in BydbQL prompt generation with natural language
support and BanyanDB schema hints. Use the list_groups_schemas tool to discover
available resources and guide your queries.');
+
+ if (TRANSPORT === 'http') {
+ const httpServer = createServer((req, res) => {
+ if (req.url !== '/mcp') {
+ res.writeHead(404, { 'Content-Type': 'application/json' });
+ res.end(JSON.stringify({ error: 'Not found' }));
+ return;
+ }
+
+ let body = '';
+ req.on('data', (chunk) => { body += chunk; });
+ req.on('end', async () => {
+ try {
+ const parsedBody = body ? JSON.parse(body) : undefined;
+ const transport = new StreamableHTTPServerTransport({
sessionIdGenerator: undefined });
+ const mcpServer = createMcpServer(banyandbClient);
+ await mcpServer.connect(transport);
+ await transport.handleRequest(req, res, parsedBody);
+ } catch (error) {
+ if (!res.headersSent) {
+ res.writeHead(500, { 'Content-Type': 'application/json' });
+ res.end(JSON.stringify({ error: error instanceof Error ?
error.message : String(error) }));
+ }
+ }
Review Comment:
`JSON.parse(body)` failures (malformed JSON) are currently caught and
returned as a 500. This is a client error and should return a 400 with a clear
message; otherwise it looks like a server fault and makes debugging harder for
MCP clients.
##########
mcp/src/index.ts:
##########
@@ -359,15 +367,67 @@ async function main() {
}
}
+ if (name === 'get_generate_bydbql_prompt') {
+ const queryHints = normalizeQueryHints(args);
+ if (!queryHints.description) {
+ throw new Error('description is required');
+ }
+
+ const { groups, resourcesByGroup } = await
loadQueryContext(banyandbClient);
+ const prompt = generateBydbQL(queryHints.description, queryHints,
groups, resourcesByGroup);
+
+ return { content: [{ type: 'text', text: prompt }] };
+ }
+
throw new Error(`Unknown tool: ${name}`);
});
- // Start the server
- const transport = new StdioServerTransport();
- await server.connect(transport);
+ return server;
+}
+
+async function main() {
+ const banyandbClient = new BanyanDBClient(BANYANDB_ADDRESS);
+
+ log.info('Using built-in BydbQL prompt generation with natural language
support and BanyanDB schema hints. Use the list_groups_schemas tool to discover
available resources and guide your queries.');
+
+ if (TRANSPORT === 'http') {
+ const httpServer = createServer((req, res) => {
+ if (req.url !== '/mcp') {
+ res.writeHead(404, { 'Content-Type': 'application/json' });
+ res.end(JSON.stringify({ error: 'Not found' }));
+ return;
+ }
Review Comment:
In HTTP transport mode the handler checks `req.url !== '/mcp'`. This will
404 for valid requests that include a query string (e.g. `/mcp?…`) which some
clients/transports may use. Prefer parsing `new URL(req.url ?? '',
'http://localhost')` and matching on `.pathname === '/mcp'`.
##########
mcp/src/index.ts:
##########
@@ -93,28 +199,55 @@ async function main() {
},
{
name: 'list_resources_bydbql',
+ description: 'Fetch streams, measures, traces, or properties from
BanyanDB using a BydbQL query.',
+ inputSchema: {
Review Comment:
`list_resources_bydbql`’s description says it fetches
streams/measures/traces/properties, but the underlying `BanyanDBClient.query()`
can also return TOPN results. Consider making the description generic (e.g.,
“execute a BydbQL query”) or explicitly include TOPN to match actual behavior.
##########
mcp/package.json:
##########
@@ -22,8 +22,7 @@
"license": "Apache-2.0",
"dependencies": {
"@modelcontextprotocol/sdk": "^1.26.0",
- "dotenv": "^16.4.5",
- "openai": "^4.20.0"
+ "dotenv": "^16.4.5"
},
Review Comment:
`src/index.ts` imports `zod`, and `@modelcontextprotocol/sdk` declares `zod`
as a non-optional peer dependency, but `package.json` doesn’t list `zod` in
dependencies. This can break installs (e.g., pnpm won’t hoist a transitive peer
the same way) and cause runtime module resolution failures. Add `zod` as an
explicit dependency (and keep `package-lock.json` in sync).
##########
mcp/src/query/llm-prompt.ts:
##########
@@ -19,10 +19,8 @@
import type { ResourcesByGroup } from './types.js';
-/**
- * Generate the LLM prompt for converting natural language to BydbQL queries.
- */
-export function generateQueryPrompt(
+// Generates a BydbQL query based on the provided natural language description
and available schema information.
+export function generateBydbQL(
description: string,
args: Record<string, unknown>,
Review Comment:
The prompt generator instructs the LLM to return JSON with a `bydbql` field,
but the tool interface (`list_resources_bydbql`) expects an input field named
`BydbQL` (capital B). This mismatch makes the “prompt → tool call” workflow
error-prone. Consider standardizing on one key (preferably `BydbQL` everywhere)
or supporting both aliases when normalizing inputs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
