Copilot commented on code in PR #26: URL: https://github.com/apache/skywalking-mcp/pull/26#discussion_r2923984139
########## internal/prompts/trace.go: ########## @@ -0,0 +1,202 @@ +// 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. + +package prompts + +import ( + "context" + "fmt" + + "github.com/mark3labs/mcp-go/mcp" +) + +func traceInvestigationHandler(_ context.Context, request mcp.GetPromptRequest) (*mcp.GetPromptResult, error) { + args := request.Params.Arguments + serviceID := args["service_id"] + traceState := args["trace_state"] + duration := args["duration"] + + if duration == "" { + duration = defaultDuration + } + if traceState == "" { + traceState = "all" + } + + // Use the dynamic tool instructions + toolInstructions := generateToolInstructions("trace_investigation") + + prompt := fmt.Sprintf(`Investigate traces with filters: service_id="%s", trace_state="%s", duration="%s". + +%s + +**Analysis Steps:** + +**Find Problematic Traces** +- First use query_traces with view="summary" to get overview +- Look for patterns in error traces, slow traces, or anomalies +- Note trace IDs that need deeper investigation + +**Deep Dive on Specific Traces** +- Use query_traces with the identified trace_id Review Comment: This prompt instructs using query_traces with a "duration" filter, but query_traces now accepts start/end/step instead. Update the prompt to produce start/end (e.g. end="now" and start derived from duration) so it matches the actual tool parameters. ```suggestion prompt := fmt.Sprintf(`Investigate traces with filters: service_id="%s", trace_state="%s", time_range="%s". %s **Analysis Steps:** **Find Problematic Traces** - First use query_traces with end="now", start derived from the requested time_range (e.g., now minus the time_range), and view="summary" to get an overview - Look for patterns in error traces, slow traces, or anomalies - Note trace IDs that need deeper investigation **Deep Dive on Specific Traces** - Use query_traces with the identified trace_id and the same start/end time window ``` ########## internal/tools/log.go: ########## @@ -65,13 +68,26 @@ func buildLogQueryCondition(req *LogQueryRequest) *api.LogQueryCondition { paging := BuildPagination(req.PageNum, req.PageSize) + order := api.OrderDes + if req.QueryOrder == "ASC" { + order = api.OrderAsc + } + cond := &api.LogQueryCondition{ ServiceID: &req.ServiceID, ServiceInstanceID: &req.ServiceInstanceID, EndpointID: &req.EndpointID, - RelatedTrace: &api.TraceScopeCondition{TraceID: req.TraceID}, QueryDuration: &duration, Paging: paging, + QueryOrder: &order, + } + + if req.TraceID != "" || req.SegmentID != "" || req.SpanID != nil { + cond.RelatedTrace = &api.TraceScopeCondition{ + TraceID: req.TraceID, + SegmentID: &req.SegmentID, + SpanID: req.SpanID, + } Review Comment: RelatedTrace always sets SegmentID to &req.SegmentID when any trace scope filter is used. When only trace_id is provided (or only span_id), this passes an empty segment_id pointer, which can unintentionally filter out all logs. Only set SegmentID when req.SegmentID != "" (and similarly avoid setting span/segment pointers unless provided). ```suggestion traceScope := &api.TraceScopeCondition{} if req.TraceID != "" { traceScope.TraceID = req.TraceID } if req.SegmentID != "" { // Only set SegmentID when it is actually provided to avoid filtering by an empty segment. traceScope.SegmentID = &req.SegmentID } if req.SpanID != nil { traceScope.SpanID = req.SpanID } cond.RelatedTrace = traceScope ``` ########## internal/prompts/trace.go: ########## @@ -0,0 +1,202 @@ +// 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. + +package prompts + +import ( + "context" + "fmt" + + "github.com/mark3labs/mcp-go/mcp" +) + +func traceInvestigationHandler(_ context.Context, request mcp.GetPromptRequest) (*mcp.GetPromptResult, error) { + args := request.Params.Arguments + serviceID := args["service_id"] + traceState := args["trace_state"] + duration := args["duration"] + + if duration == "" { + duration = defaultDuration + } + if traceState == "" { + traceState = "all" + } + + // Use the dynamic tool instructions + toolInstructions := generateToolInstructions("trace_investigation") + + prompt := fmt.Sprintf(`Investigate traces with filters: service_id="%s", trace_state="%s", duration="%s". + +%s + +**Analysis Steps:** + +**Find Problematic Traces** +- First use query_traces with view="summary" to get overview +- Look for patterns in error traces, slow traces, or anomalies +- Note trace IDs that need deeper investigation + +**Deep Dive on Specific Traces** +- Use query_traces with the identified trace_id +- Start with view="summary" for quick insights +- Use view="full" for complete span analysis +- Use view="errors_only" if focusing on errors + +**Performance Analysis** +- Look for traces with high duration using min_trace_duration filter +- Identify bottlenecks in span timings +- Check for cascading delays + +**Error Pattern Analysis** +- Use query_traces with trace_state="error" +- Group errors by type and service +- Identify error propagation paths + +Provide specific findings and actionable recommendations.`, serviceID, traceState, duration, toolInstructions) + + return &mcp.GetPromptResult{ + Description: "Trace investigation using query tools", + Messages: []mcp.PromptMessage{ + { + Role: mcp.RoleUser, + Content: mcp.TextContent{ + Type: "text", + Text: prompt, + }, + }, + }, + }, nil +} + +func logAnalysisHandler(_ context.Context, request mcp.GetPromptRequest) (*mcp.GetPromptResult, error) { + args := request.Params.Arguments + serviceID := args["service_id"] + logLevel := args["log_level"] + duration := args["duration"] + + if duration == "" { + duration = defaultDuration + } + if logLevel == "" { + logLevel = "ERROR" + } + + prompt := fmt.Sprintf(`Analyze service logs using the query_logs tool: + +**Tool Configuration:** +- query_logs with following parameters: + - service_id: "%s" (if specified) + - tags: [{"key": "level", "value": "%s"}] for log level filtering + - duration: "%s" for time range Review Comment: This prompt tells users to call query_logs with a "duration" parameter, but query_logs only supports start/end/step. Please update the tool configuration section to use start/end (or call the generate_duration prompt first) so the instructions are actionable. ```suggestion - start and end: timestamps defining the time range (e.g., last %s) ``` ########## internal/prompts/trace.go: ########## @@ -0,0 +1,202 @@ +// 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. + +package prompts + +import ( + "context" + "fmt" + + "github.com/mark3labs/mcp-go/mcp" +) + +func traceInvestigationHandler(_ context.Context, request mcp.GetPromptRequest) (*mcp.GetPromptResult, error) { + args := request.Params.Arguments + serviceID := args["service_id"] + traceState := args["trace_state"] + duration := args["duration"] + + if duration == "" { + duration = defaultDuration + } + if traceState == "" { + traceState = "all" + } + + // Use the dynamic tool instructions + toolInstructions := generateToolInstructions("trace_investigation") + + prompt := fmt.Sprintf(`Investigate traces with filters: service_id="%s", trace_state="%s", duration="%s". + +%s + +**Analysis Steps:** + +**Find Problematic Traces** +- First use query_traces with view="summary" to get overview +- Look for patterns in error traces, slow traces, or anomalies +- Note trace IDs that need deeper investigation + +**Deep Dive on Specific Traces** +- Use query_traces with the identified trace_id +- Start with view="summary" for quick insights +- Use view="full" for complete span analysis +- Use view="errors_only" if focusing on errors + +**Performance Analysis** +- Look for traces with high duration using min_trace_duration filter +- Identify bottlenecks in span timings +- Check for cascading delays + +**Error Pattern Analysis** +- Use query_traces with trace_state="error" +- Group errors by type and service +- Identify error propagation paths + +Provide specific findings and actionable recommendations.`, serviceID, traceState, duration, toolInstructions) + + return &mcp.GetPromptResult{ + Description: "Trace investigation using query tools", + Messages: []mcp.PromptMessage{ + { + Role: mcp.RoleUser, + Content: mcp.TextContent{ + Type: "text", + Text: prompt, + }, + }, + }, + }, nil +} + +func logAnalysisHandler(_ context.Context, request mcp.GetPromptRequest) (*mcp.GetPromptResult, error) { + args := request.Params.Arguments + serviceID := args["service_id"] + logLevel := args["log_level"] + duration := args["duration"] + + if duration == "" { + duration = defaultDuration + } + if logLevel == "" { + logLevel = "ERROR" + } + + prompt := fmt.Sprintf(`Analyze service logs using the query_logs tool: + +**Tool Configuration:** +- query_logs with following parameters: + - service_id: "%s" (if specified) + - tags: [{"key": "level", "value": "%s"}] for log level filtering + - duration: "%s" for time range + - cold: true if historical data needed + +**Analysis Steps:** + +**Log Pattern Analysis** +- Use query_logs to get recent logs for the service +- Filter by log level (ERROR, WARN, INFO) +- Look for recurring error patterns +- Identify frequency of different log types + +**Error Investigation** +- Focus on ERROR level logs first +- Group similar error messages +- Check for correlation with trace IDs +- Look for timestamp patterns + +**Performance Correlation** +- Compare log timestamps with performance issues +- Look for resource exhaustion indicators +- Check for timeout or connection errors + +**Troubleshooting Workflow** +- Start with ERROR logs in the specified time range +- Use trace_id from logs to get detailed trace analysis +- Cross-reference with metrics for full picture + +Provide specific log analysis findings and recommendations.`, serviceID, logLevel, duration) + + return &mcp.GetPromptResult{ + Description: "Log analysis using query_logs tool", + Messages: []mcp.PromptMessage{ + { + Role: mcp.RoleUser, + Content: mcp.TextContent{ + Type: "text", + Text: prompt, + }, + }, + }, + }, nil +} + +func traceDeepDiveHandler(_ context.Context, request mcp.GetPromptRequest) (*mcp.GetPromptResult, error) { + args := request.Params.Arguments + traceID := args["trace_id"] + view := args["view"] + + if view == "" { + view = "summary" + } Review Comment: traceDeepDiveHandler ignores the prompt argument "check_cold_storage" (it isn't read from request.Params.Arguments). Either remove that argument from the prompt registration or implement the cold-storage fallback instructions in the generated prompt text. ########## internal/tools/log.go: ########## @@ -110,20 +127,35 @@ Workflow: Examples: - {"service_id": "Your_ApplicationName", "start": "2024-06-01 12:00:00", "end": "2024-06-01 13:00:00"}: Query logs for a service in a time range - {"trace_id": "abc123..."}: Query logs related to a specific trace +- {"trace_id": "abc123...", "segment_id": "seg456...", "span_id": 0}: Query logs for a specific span - {"tags": [{"key": "level", "value": "ERROR"}], "cold": true}: Query error logs from cold storage`, Review Comment: The query_logs docstring still references using a "duration" parameter to limit the time range, but the tool API only supports start/end/step. Please update the tool description/examples so callers don't try to pass an unsupported duration field. ########## README.md: ########## @@ -86,12 +86,9 @@ If using Docker: "run", "--rm", "-i", - "-e", - "SW_URL", - "skywalking-mcp:latest" - ], - "env": { - "SW_URL": "http://localhost:12800"; + "skywalking-mcp:latest", + "--sw-url", + "http://localhost:12800"; } Review Comment: The Docker + Cursor configuration JSON is malformed: the "args" array isn't closed (it's closed with a "}" instead of a "]"). As written, the snippet can't be pasted into a JSON config file. ```suggestion ] ``` ########## README.md: ########## @@ -102,17 +99,41 @@ If using Docker: SkyWalking MCP provides the following tools to query and analyze SkyWalking OAP data: -| Category | Tool Name | Description | Key Features | -|-------------|--------------------------|----------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| **Trace** | `get_trace_details` | Get detailed trace information | Retrieve trace by ID; **Multiple views**: `full` (complete trace), `summary` (overview with metrics), `errors_only` (error spans only); Detailed span analysis | -| **Trace** | `get_cold_trace_details` | Get trace details from cold storage | Query historical traces from BanyanDB; **Multiple views**: `full`, `summary`, `errors_only`; Duration-based search; Historical incident investigation | -| **Trace** | `query_traces` | Query traces with intelligent analysis | Multi-condition filtering (service, endpoint, duration, state, tags); **Multiple views**: `full` (raw data), `summary` (intelligent analysis with performance insights), `errors_only` (error traces); Sort options; Slow trace detection; Performance metrics and statistics | -| **Metrics** | `query_single_metrics` | Query single metric values | Get specific metric values (CPM, response time, SLA, Apdex); Multiple entity scopes (Service, ServiceInstance, Endpoint, Process, Relations); Time range and cold storage support | -| **Metrics** | `query_top_n_metrics` | Query top N metric rankings | Rank entities by metric values; Configurable top N count; Ascending/descending order; Scope-based filtering; Performance analysis and issue identification | -| **Log** | `query_logs` | Query logs from SkyWalking OAP | Filter by service, instance, endpoint, trace ID, tags; Time range queries; Cold storage support; Pagination support | -| **MQE** | `execute_mqe_expression` | Execute MQE expressions for metrics | Execute complex MQE (Metrics Query Expression) queries; Support calculations, aggregations, comparisons, TopN, trend analysis; Multiple result types (single value, time series, sorted list); Entity filtering and relation metrics; Debug and tracing capabilities | -| **MQE** | `list_mqe_metrics` | List available metrics for MQE | Discover available metrics for MQE queries; Filter by regex patterns; Get metric metadata (type, catalog); Support service, instance, endpoint, relation, database, and infrastructure metrics | -| **MQE** | `get_mqe_metric_type` | Get metric type information | Get detailed type information for specific metrics; Understand metric structure (regular value, labeled value, sampled record); Help with correct MQE expression syntax | +| Category | Tool Name | Description | +|--------------|--------------------------------|---------------------------------------------------------------------------------------------------| +| **Trace** | `query_traces` | Query traces with multi-condition filtering (service, endpoint, duration, state, tags). Supports `full`, `summary`, and `errors_only` views with performance insights. | Review Comment: This description still mentions filtering traces by "duration", but the query_traces tool now uses start/end/step for time ranges. Update the README so it matches the current tool parameters. ```suggestion | **Trace** | `query_traces` | Query traces with multi-condition filtering (service, endpoint, state, tags, and time range via start/end/step). Supports `full`, `summary`, and `errors_only` views with performance insights. | ``` ########## internal/prompts/analysis.go: ########## @@ -0,0 +1,238 @@ +// 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. + +package prompts + +import ( + "context" + "fmt" + + "github.com/mark3labs/mcp-go/mcp" +) + +func performanceAnalysisHandler(_ context.Context, request mcp.GetPromptRequest) (*mcp.GetPromptResult, error) { + args := request.Params.Arguments + serviceName := args["service_name"] + duration := args["duration"] + + if duration == "" { + duration = defaultDuration + } + + // Use the dynamic tool instructions + toolInstructions := generateToolInstructions("performance_analysis") + + prompt := fmt.Sprintf(`Please analyze the performance of service '%s' over the last %s. + +%s + Review Comment: The analyze-performance prompt takes a "duration" argument, but the tools it references (execute_mqe_expression/query_traces) use start/end/step rather than duration. Consider converting duration into start/end inside the handler (or instruct to use generate_duration) so the prompt leads to valid tool calls. ########## internal/prompts/tools.go: ########## @@ -0,0 +1,91 @@ +// 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. + +package prompts + +import "fmt" + +// Tool capability mapping for different analysis types +var toolCapabilities = map[string][]string{ + "performance_analysis": { + "execute_mqe_expression", Review Comment: toolCapabilities for "performance_analysis" lists only execute_mqe_expression, but analysisChains for that analysis type recommends calling query_traces too. This makes generateToolInstructions internally inconsistent; add query_traces to toolCapabilities (or remove it from the chain). ```suggestion "execute_mqe_expression", "query_traces", ``` -- 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]
