Copilot commented on code in PR #870:
URL:
https://github.com/apache/skywalking-banyandb/pull/870#discussion_r2570395905
##########
mcp/src/index.ts:
##########
@@ -216,8 +216,44 @@ async function main() {
let bydbqlQueryResult: QueryGeneratorResult;
try {
+ // Fetch groups from BanyanDB before generating query
+ let groups: string[] = [];
+ try {
+ const groupsList = await banyandbClient.listGroups();
+ groups = groupsList.map((g) => g.metadata?.name || '').filter((n) =>
n !== '');
+ } catch (error) {
+ log.warn('Failed to fetch groups, continuing without group
information:', error instanceof Error ? error.message : String(error));
+ }
+
+ // Fetch resources from all groups before generating query
+ const resourcesByGroup: ResourcesByGroup = {};
+ for (const group of groups) {
+ try {
+ const [streams, measures, traces, properties, topNItems,
indexRule] = await Promise.all([
+ banyandbClient.listStreams(group).catch(() => []),
+ banyandbClient.listMeasures(group).catch(() => []),
+ banyandbClient.listTraces(group).catch(() => []),
+ banyandbClient.listProperties(group).catch(() => []),
+ banyandbClient.listTopN(group).catch(() => []),
+ banyandbClient.listIndexRule(group).catch(() => []),
+ ]);
+
+ resourcesByGroup[group] = {
+ streams: streams.map((r) => r.metadata?.name || '').filter((n)
=> n !== ''),
+ measures: measures.map((r) => r.metadata?.name || '').filter((n)
=> n !== ''),
+ traces: traces.map((r) => r.metadata?.name || '').filter((n) =>
n !== ''),
+ properties: properties.map((r) => r.metadata?.name ||
'').filter((n) => n !== ''),
+ topNItems: topNItems.map((r) => r.metadata?.name ||
'').filter((n) => n !== ''),
+ indexRule: indexRule.filter((r) => !r.noSort &&
r.metadata?.name).map((r) => r.metadata?.name || '')
Review Comment:
Missing semicolon at the end of the statement. This line should end with a
semicolon for consistency with the rest of the codebase and proper
JavaScript/TypeScript syntax.
##########
mcp/src/client/types.ts:
##########
@@ -0,0 +1,110 @@
+/**
+ * Licensed to Apache Software Foundation (ASF) under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Apache Software Foundation (ASF) licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+interface TagValue {
+ str?: { value: string };
+ int?: { value: number };
+ float?: { value: number };
+ binaryData?: unknown;
+}
+
+interface Tag {
+ key: string;
+ value: TagValue;
+}
+
+interface TagFamily {
+ tags?: Tag[];
+}
+
+interface FieldValue {
+ int?: { value: number };
+ float?: { value: number };
+ str?: { value: string };
+ binaryData?: unknown;
+}
+
+interface Field {
+ name: string;
+ value: FieldValue;
+}
+
+interface DataPoint {
+ timestamp?: string | number;
+ sid?: string;
+ version?: string | number;
+ tagFamilies?: TagFamily[];
+ fields?: Field[];
+}
+
+interface StreamResult {
+ elements?: unknown[];
+}
+
+interface MeasureResult {
+ dataPoints?: DataPoint[];
+ data_points?: DataPoint[];
+}
+
+interface TraceResult {
+ elements?: unknown[];
+}
+
+interface PropertyResult {
+ items?: unknown[];
+}
+
+interface TopNResult {
+ lists?: unknown[];
+}
+
+export interface QueryRequest {
+ query: string;
+}
+
+export interface QueryResponse {
+ // Response can be either wrapped in result or direct
+ result?: {
+ streamResult?: StreamResult;
+ measureResult?: MeasureResult;
+ traceResult?: TraceResult;
+ propertyResult?: PropertyResult;
+ topnResult?: TopNResult;
+ };
+ // Or directly at top level
+ streamResult?: StreamResult;
+ measureResult?: MeasureResult;
+ traceResult?: TraceResult;
+ propertyResult?: PropertyResult;
+ topnResult?: TopNResult;
+}
+
+export interface Group {
+ metadata?: {
+ name?: string;
+ };
+}
+
+export interface ResourceMetadata {
+ metadata?: {
+ name?: string;
+ group?: string;
+ };
+ noSort: boolean;
Review Comment:
The `noSort` property should be optional. Index rules and other resource
types may not always have this field, and marking it as required could cause
type errors. Change to `noSort?: boolean;` to make it optional.
```suggestion
noSort?: boolean;
```
##########
mcp/src/index.ts:
##########
@@ -320,3 +377,5 @@ main().catch((error) => {
}
process.exit(1);
});
+
+
Review Comment:
Extra blank lines at the end of the file. Remove these trailing blank lines
to maintain consistency with code formatting standards.
```suggestion
```
##########
mcp/src/utils/http.ts:
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to Apache Software Foundation (ASF) under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Apache Software Foundation (ASF) licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+const Timeout = 2 * 60 * 1000;
+
+export let globalAbortController = new AbortController();
+
+export function abortRequestsAndUpdate() {
+ globalAbortController.abort(`Request timeout ${Timeout}ms`);
+ globalAbortController = new AbortController();
+}
+
+export async function httpFetch({
+ url = '',
+ method = 'GET',
+ json,
+ headers = {},
+}: {
+ method: string;
+ json: unknown;
+ headers?: Record<string, string>;
+ url: string;
+}) {
+ const timeoutId = setTimeout(() => {
+ abortRequestsAndUpdate();
+ }, Timeout);
+
+ // Only include body and Content-Type for requests that have a body
+ const hasBody = json !== null && json !== undefined && method !== 'GET' &&
method !== 'HEAD';
+ const requestHeaders: Record<string, string> = { ...headers };
+ if (hasBody) {
+ requestHeaders['Content-Type'] = 'application/json';
+ }
+
+ const response: Response = await fetch(url, {
+ method,
+ headers: requestHeaders,
+ body: hasBody ? JSON.stringify(json) : undefined,
+ signal: globalAbortController.signal,
+ })
+ .catch((error) => {
+ throw error;
+ })
+ .finally(() => {
+ clearTimeout(timeoutId);
+ });
+ if (response.ok) {
+ return response.json();
+ } else {
+ console.error(response);
Review Comment:
Using `console.error` directly here is inconsistent with the logging
approach used elsewhere in the codebase. Consider using the `log.error()`
helper from `utils/logger.ts` for consistent logging across the application.
##########
mcp/src/query/query-generator.ts:
##########
@@ -0,0 +1,298 @@
+/**
+ * Licensed to Apache Software Foundation (ASF) under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Apache Software Foundation (ASF) licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import OpenAI from 'openai';
+import { generateQueryPrompt } from './llm-prompt.js';
+import { PatternMatcher } from './pattern-matcher.js';
+import type { QueryGeneratorResult, ResourcesByGroup } from './types.js';
+
+/**
+ * QueryGenerator converts natural language descriptions to BydbQL queries.
+ * Supports both LLM-based generation (when API key is provided) and
pattern-based fallback.
+ */
+export class QueryGenerator {
+ private static readonly OPENAI_API_TIMEOUT_MS = 20000; // 20 seconds timeout
for LLM API calls
+
+ private openaiClient: OpenAI | null = null;
+ private patternMatcher: PatternMatcher;
+
+ constructor(apiKey?: string, baseURL?: string) {
+ // Validate API key format before creating client
+ if (apiKey && apiKey.trim().length > 0) {
+ const trimmedKey = apiKey.trim();
+ if (trimmedKey.length < 10) {
+ console.error('[QueryGenerator] Warning: API key appears to be too
short. LLM query generation may fail.');
+ }
+ this.openaiClient = new OpenAI({
+ apiKey: trimmedKey,
+ ...(baseURL && { baseURL }),
+ });
+ }
+ this.patternMatcher = new PatternMatcher();
+ }
+
+ /**
+ * Generate a BydbQL query from a natural language description.
+ */
+ async generateQuery(
+ description: string,
+ args: Record<string, unknown>,
+ groups: string[] = [],
+ resourcesByGroup: ResourcesByGroup = {},
+ ): Promise<QueryGeneratorResult> {
+ // Use LLM if available, otherwise fall back to pattern matching
+ if (this.openaiClient) {
+ try {
+ return await this.generateQueryWithLLM(description, args, groups,
resourcesByGroup);
+ } catch (error: unknown) {
+ // Check for API key authentication errors
+ const errorObj = error as { status?: number; message?: string };
+ if (
+ errorObj?.status === 401 ||
+ errorObj?.message?.includes('401') ||
+ errorObj?.message?.includes('Invalid API key')
+ ) {
+ console.error('[QueryGenerator] API key authentication failed.
Falling back to pattern-based generation.');
+ console.error('[QueryGenerator] Error details:', errorObj.message ||
String(error));
+ // Disable LLM client to prevent repeated failures
+ this.openaiClient = null;
+ } else {
+ // For other errors (timeout, network, etc.), log but don't disable
+ console.error('[QueryGenerator] Error generating query with LLM:',
errorObj.message || String(error));
+ }
+ // Fall through to pattern-based generation
+ }
+ }
+ return this.generateQueryWithPatterns(description, args);
+ }
+
+ /**
+ * Generate query using LLM (OpenAI).
+ */
+ private async generateQueryWithLLM(
+ description: string,
+ args: Record<string, unknown>,
+ groups: string[] = [],
+ resourcesByGroup: ResourcesByGroup = {},
+ ): Promise<QueryGeneratorResult> {
+ if (!this.openaiClient) {
+ throw new Error('OpenAI client not initialized');
+ }
+ const prompt = generateQueryPrompt(description, args, groups,
resourcesByGroup);
+
+ const completion = await Promise.race([
+ this.openaiClient.chat.completions.create({
+ model: 'gpt-4o-mini',
+ messages: [
+ {
+ role: 'system',
+ content:
+ 'You are a BydbQL query generator. Return a JSON object with the
following fields: bydbql (the BydbQL query), group (group name), name (resource
name), type (resource type), and explanations (brief explanation of the
query).',
+ },
+ {
+ role: 'user',
+ content: prompt,
+ },
+ ],
+ response_format: { type: 'json_object' },
+ }),
+ new Promise<never>((_, reject) =>
+ setTimeout(
+ () => reject(new Error(`LLM API timeout after
${QueryGenerator.OPENAI_API_TIMEOUT_MS / 1000} seconds`)),
+ QueryGenerator.OPENAI_API_TIMEOUT_MS,
+ ),
+ ),
+ ]);
+
+ const responseContent = completion.choices[0]?.message?.content?.trim();
+ if (!responseContent) {
+ throw new Error('Empty response from LLM');
+ }
+
+ // Parse JSON response
+ let parsedResponse: {
+ bydbql?: string;
+ group?: string;
+ name?: string;
+ type?: string;
+ explanations?: string;
+ };
+
+ try {
+ parsedResponse = JSON.parse(responseContent);
+ } catch (error) {
+ console.error('JSON parsing failed:', error);
Review Comment:
Using `console.error` directly here is inconsistent with the logging
approach used elsewhere in the codebase. Consider using the `log.error()`
helper from `utils/logger.ts` for consistent logging across the application.
##########
mcp/src/utils/http.ts:
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to Apache Software Foundation (ASF) under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Apache Software Foundation (ASF) licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+const Timeout = 2 * 60 * 1000;
+
+export let globalAbortController = new AbortController();
+
+export function abortRequestsAndUpdate() {
+ globalAbortController.abort(`Request timeout ${Timeout}ms`);
+ globalAbortController = new AbortController();
+}
+
+export async function httpFetch({
+ url = '',
+ method = 'GET',
+ json,
+ headers = {},
+}: {
+ method: string;
+ json: unknown;
+ headers?: Record<string, string>;
+ url: string;
+}) {
+ const timeoutId = setTimeout(() => {
+ abortRequestsAndUpdate();
+ }, Timeout);
+
+ // Only include body and Content-Type for requests that have a body
+ const hasBody = json !== null && json !== undefined && method !== 'GET' &&
method !== 'HEAD';
+ const requestHeaders: Record<string, string> = { ...headers };
+ if (hasBody) {
+ requestHeaders['Content-Type'] = 'application/json';
+ }
+
+ const response: Response = await fetch(url, {
+ method,
+ headers: requestHeaders,
+ body: hasBody ? JSON.stringify(json) : undefined,
+ signal: globalAbortController.signal,
+ })
+ .catch((error) => {
+ throw error;
+ })
Review Comment:
This catch block is redundant - it simply re-throws the caught error without
any processing. Consider removing the `.catch()` entirely or add meaningful
error handling/logging if needed.
```suggestion
```
--
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]
