This is an automated email from the ASF dual-hosted git repository. wu-sheng pushed a commit to branch fix/trace-query-v1-fallback in repository https://gitbox.apache.org/repos/asf/skywalking-mcp.git
commit a6552f857e9589a2142888b845f29924c76c83ca Author: Wu Sheng <[email protected]> AuthorDate: Mon Jul 6 16:46:30 2026 +0800 fix: support v1 trace query protocol for non-BanyanDB storages The query_traces tool only spoke the trace-v2 GraphQL API (queryTraces), which is BanyanDB-only. On Elasticsearch/JDBC storages and older OAP releases it failed with "Field 'queryTraces' is undefined" or "Only BanyanDB storage support Trace V2 query now". Detect backend support via the hasQueryTracesV2Support field and fall back to the legacy queryBasicTraces + queryTrace (v1) protocol, assembling the result into the same TraceList shape so downstream processing is identical. The queryTrace `duration` argument is omitted deliberately: it is BanyanDB-only and absent on OAP < 10.3.0, so including it would break the fallback on exactly the older backends it targets. Fixes apache/skywalking#13939 Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> --- internal/tools/trace.go | 124 ++++++++++++++++++++++++++++- internal/tools/trace_test.go | 180 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 302 insertions(+), 2 deletions(-) diff --git a/internal/tools/trace.go b/internal/tools/trace.go index 208a17c..68a1806 100644 --- a/internal/tools/trace.go +++ b/internal/tools/trace.go @@ -101,6 +101,119 @@ func tracesV2(ctx context.Context, condition *api.TraceQueryCondition) (api.Trac return response["result"], err } +// hasQueryTracesV2SupportGQL detects whether the backend supports the trace-v2 query API. +const hasQueryTracesV2SupportGQL = ` +query { + result: hasQueryTracesV2Support +}` + +// queryBasicTracesV1GQL is the GraphQL query for the trace-v1 basic-traces protocol. +const queryBasicTracesV1GQL = ` +query ($condition: TraceQueryCondition!) { + result: queryBasicTraces(condition: $condition) { + traces { segmentId endpointNames duration start isError traceIds } + } +}` + +// queryTraceV1GQL fetches the full spans of a single trace (trace-v1 protocol). +// The queryTrace `duration` argument is omitted deliberately: it is BanyanDB-only and +// absent on OAP < 10.3.0, so including it would break this fallback on older backends. +const queryTraceV1GQL = ` +query ($traceId: ID!) { + result: queryTrace(traceId: $traceId) { + spans { + traceId segmentId spanId parentSpanId + refs { traceId parentSegmentId parentSpanId type } + serviceCode serviceInstanceName + startTime endTime endpointName type peer component isError layer + tags { key value } + logs { time data { key value } } + attachedEvents { + startTime { seconds nanos } event endTime { seconds nanos } + tags { key value } summary { key value } + } + } + } +}` + +// queryTracesAuto runs a trace query using the protocol the backend supports: the +// queryTraces (v2) API for BanyanDB, or the legacy v1 protocol for Elasticsearch/JDBC +// and older OAP releases. +func queryTracesAuto(ctx context.Context, condition *api.TraceQueryCondition) (api.TraceList, error) { + if supportsTraceV2(ctx) { + return tracesV2(ctx, condition) + } + return tracesV1(ctx, condition) +} + +// supportsTraceV2 reports whether the backend supports the trace-v2 API. Any error +// (field absent on an older OAP, or an unreachable backend) counts as "not supported" +// so the v1 path is used; a genuinely broken connection surfaces on the following query. +func supportsTraceV2(ctx context.Context) bool { + var response map[string]bool + request := graphql.NewRequest(hasQueryTracesV2SupportGQL) + if err := client.ExecuteQuery(ctx, request, &response); err != nil { + return false + } + return response["result"] +} + +// basicTracesV1 queries the trace summaries using the queryBasicTraces (v1) protocol. +func basicTracesV1(ctx context.Context, condition *api.TraceQueryCondition) (api.TraceBrief, error) { + var response map[string]api.TraceBrief + request := graphql.NewRequest(queryBasicTracesV1GQL) + request.Var("condition", condition) + err := client.ExecuteQuery(ctx, request, &response) + return response["result"], err +} + +// traceV1 fetches the full spans of a single trace using the queryTrace (v1) protocol. +func traceV1(ctx context.Context, traceID string) (api.Trace, error) { + var response map[string]api.Trace + request := graphql.NewRequest(queryTraceV1GQL) + request.Var("traceId", traceID) + err := client.ExecuteQuery(ctx, request, &response) + return response["result"], err +} + +// tracesV1 runs the legacy v1 protocol and assembles the result into the same TraceList +// shape as v2 so downstream processing is identical: queryBasicTraces returns only +// summaries, so each unique trace's spans are fetched via queryTrace. +// +// On non-BanyanDB storages queryBasicTraces paginates over segments, so page_size bounds +// segments, not traces; segments of the same trace are de-duplicated here by trace ID, so +// a broad query may yield fewer than page_size traces (narrow by service/endpoint). +func tracesV1(ctx context.Context, condition *api.TraceQueryCondition) (api.TraceList, error) { + brief, err := basicTracesV1(ctx, condition) + if err != nil { + return api.TraceList{}, err + } + + traceList := api.TraceList{Traces: make([]*api.TraceV2, 0, len(brief.Traces))} + seen := make(map[string]struct{}, len(brief.Traces)) + for _, basic := range brief.Traces { + if basic == nil || len(basic.TraceIds) == 0 { + continue + } + traceID := basic.TraceIds[0] + if _, ok := seen[traceID]; ok { + continue + } + seen[traceID] = struct{}{} + + trace, err := traceV1(ctx, traceID) + if err != nil { + return api.TraceList{}, err + } + if len(trace.Spans) == 0 { + // Trace vanished between the two calls; skip so it is not counted as empty. + continue + } + traceList.Traces = append(traceList.Traces, &api.TraceV2{Spans: trace.Spans}) + } + return traceList, nil +} + // Trace-specific constants const ( DefaultTracePageSize = 20 @@ -393,8 +506,10 @@ func searchTraces(ctx context.Context, req *TracesQueryRequest) (*mcp.CallToolRe return mcp.NewToolResultError(err.Error()), nil } - // Execute query using queryTraces (v2 protocol) - traceList, err := tracesV2(ctx, condition) + // Execute the query, automatically selecting the trace protocol supported by + // the backend storage: queryTraces (v2) for BanyanDB, or the legacy + // queryBasicTraces + queryTrace (v1) for Elasticsearch/JDBC and older OAP. + traceList, err := queryTracesAuto(ctx, condition) if err != nil { return mcp.NewToolResultError(fmt.Sprintf(ErrFailedToQueryTraces, err)), nil } @@ -608,6 +723,11 @@ Important Notes: - SkyWalking OAP requires either a time range or 'trace_id' to be specified - If no time range and no trace_id are provided, a default duration of "1h" (last 1 hour) will be used - This ensures the query always has a valid time range or specific trace to search +- The trace query protocol is selected automatically based on the backend storage, so this + tool works with BanyanDB, Elasticsearch, and JDBC storages +- On non-BanyanDB storages, page_size bounds trace segments rather than whole traces, so a + broad query may return fewer distinct traces than page_size; narrow it by service or + endpoint for more complete results View Options: - 'full': (Default) Complete raw data for detailed analysis diff --git a/internal/tools/trace_test.go b/internal/tools/trace_test.go new file mode 100644 index 0000000..7beaef3 --- /dev/null +++ b/internal/tools/trace_test.go @@ -0,0 +1,180 @@ +// 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 tools + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/mark3labs/mcp-go/mcp" + api "skywalking.apache.org/repo/goapi/query" + + "github.com/apache/skywalking-cli/pkg/contextkey" +) + +// fakeOAPServer emulates the OAP GraphQL endpoint for trace queries. It routes on +// the query text: the v2-support probe, the v1 basic-traces query, the v1 +// single-trace query, and the v2 queryTraces query. +func fakeOAPServer(t *testing.T, v2Supported bool) *httptest.Server { + t.Helper() + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var body struct { + Query string `json:"query"` + Variables map[string]interface{} `json:"variables"` + } + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + w.Header().Set("Content-Type", "application/json") + + switch { + case strings.Contains(body.Query, "hasQueryTracesV2Support"): + fmt.Fprintf(w, `{"data":{"result":%t}}`, v2Supported) + case strings.Contains(body.Query, "queryBasicTraces"): + // Three summaries referencing two unique trace IDs (t1 appears twice). + _, _ = w.Write([]byte(`{"data":{"result":{"traces":[` + + `{"segmentId":"s1","endpointNames":["/a"],"duration":100,"start":"1000","isError":false,"traceIds":["t1"]},` + + `{"segmentId":"s2","endpointNames":["/a"],"duration":100,"start":"1000","isError":false,"traceIds":["t1"]},` + + `{"segmentId":"s3","endpointNames":["/b"],"duration":200,"start":"2000","isError":true,"traceIds":["t2"]}` + + `]}}}`)) + case strings.Contains(body.Query, "queryTraces("): + // v2 path. + _, _ = w.Write([]byte(`{"data":{"result":{"traces":[{"spans":[` + + `{"traceId":"v2","segmentId":"seg","spanId":0,"parentSpanId":-1,"serviceCode":"svcV2",` + + `"startTime":10,"endTime":20,"endpointName":"/v2","type":"Entry","isError":false}` + + `]}],"retrievedTimeRange":{"startTime":0,"endTime":0}}}}`)) + case strings.Contains(body.Query, "queryTrace("): + traceID, _ := body.Variables["traceId"].(string) + svc, ep, isErr, st, et := "svcA", "/a", "false", 1000, 1100 + if traceID == "t2" { + svc, ep, isErr, st, et = "svcB", "/b", "true", 2000, 2200 + } + fmt.Fprintf(w, `{"data":{"result":{"spans":[`+ + `{"traceId":%q,"segmentId":"seg","spanId":0,"parentSpanId":-1,"serviceCode":%q,`+ + `"serviceInstanceName":"i","startTime":%d,"endTime":%d,"endpointName":%q,"type":"Entry",`+ + `"isError":%s,"refs":[],"tags":[],"logs":[],"attachedEvents":[]}`+ + `]}}}`, traceID, svc, st, et, ep, isErr) + default: + http.Error(w, "unexpected query", http.StatusBadRequest) + } + })) +} + +// traceTestContext builds a context carrying the base URL and (required) insecure flag +// expected by the skywalking-cli GraphQL client. +func traceTestContext(url string) context.Context { + ctx := context.WithValue(context.Background(), contextkey.BaseURL{}, url) + return context.WithValue(ctx, contextkey.Insecure{}, false) +} + +func TestSupportsTraceV2(t *testing.T) { + tsTrue := fakeOAPServer(t, true) + defer tsTrue.Close() + if !supportsTraceV2(traceTestContext(tsTrue.URL)) { + t.Fatal("expected v2 support to be true") + } + + tsFalse := fakeOAPServer(t, false) + defer tsFalse.Close() + if supportsTraceV2(traceTestContext(tsFalse.URL)) { + t.Fatal("expected v2 support to be false") + } + + // An unreachable backend must degrade to "not supported" so the v1 path is used. + if supportsTraceV2(traceTestContext("http://127.0.0.1:1";)) { + t.Fatal("expected v2 support to be false on connection error") + } +} + +func TestQueryTracesAutoUsesV1WhenV2Unsupported(t *testing.T) { + ts := fakeOAPServer(t, false) + defer ts.Close() + + list, err := queryTracesAuto(traceTestContext(ts.URL), &api.TraceQueryCondition{}) + if err != nil { + t.Fatalf("queryTracesAuto returned error: %v", err) + } + + // Three basic-trace rows collapse to two unique traces (t1 is deduplicated). + if len(list.Traces) != 2 { + t.Fatalf("expected 2 deduplicated traces, got %d", len(list.Traces)) + } + for _, tr := range list.Traces { + if len(tr.Spans) == 0 { + t.Fatal("expected each assembled trace to carry its spans from queryTrace") + } + } + + // The summary is derived from the fetched spans, including the error state. + result, err := processTracesResult(&list, ViewSummary, 0) + if err != nil { + t.Fatalf("processTracesResult returned error: %v", err) + } + if result.IsError { + t.Fatalf("unexpected tool error: %v", result) + } + text, ok := result.Content[0].(mcp.TextContent) + if !ok { + t.Fatalf("unexpected content type: %T", result.Content[0]) + } + var summary TracesSummary + if err := json.Unmarshal([]byte(text.Text), &summary); err != nil { + t.Fatalf("failed to decode summary: %v", err) + } + if summary.TotalTraces != 2 { + t.Fatalf("total_traces = %d, want 2", summary.TotalTraces) + } + if summary.ErrorCount != 1 { + t.Fatalf("error_count = %d, want 1", summary.ErrorCount) + } + if summary.SuccessCount != 1 { + t.Fatalf("success_count = %d, want 1", summary.SuccessCount) + } +} + +// TestQueryTraceV1OmitsDurationArgument guards against re-introducing the queryTrace +// `duration` argument, which does not exist on OAP releases older than 10.3.0 (and is +// BanyanDB-only where it does), and would break the v1 fallback on the exact older, +// non-BanyanDB backends it targets. +func TestQueryTraceV1OmitsDurationArgument(t *testing.T) { + if strings.Contains(queryTraceV1GQL, "duration") { + t.Fatal("queryTraceV1GQL must not reference the duration argument (breaks OAP < 10.3.0)") + } +} + +func TestQueryTracesAutoUsesV2WhenSupported(t *testing.T) { + ts := fakeOAPServer(t, true) + defer ts.Close() + + list, err := queryTracesAuto(traceTestContext(ts.URL), &api.TraceQueryCondition{}) + if err != nil { + t.Fatalf("queryTracesAuto returned error: %v", err) + } + if len(list.Traces) != 1 { + t.Fatalf("expected 1 trace from v2 path, got %d", len(list.Traces)) + } + if len(list.Traces[0].Spans) == 0 || list.Traces[0].Spans[0].ServiceCode != "svcV2" { + t.Fatalf("expected v2 span with serviceCode svcV2, got %+v", list.Traces[0].Spans) + } +}
