This is an automated email from the ASF dual-hosted git repository.
wu-sheng pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/skywalking-mcp.git
The following commit(s) were added to refs/heads/main by this push:
new 9827da7 fix: support v1 trace query protocol for non-BanyanDB
storages (#43)
9827da7 is described below
commit 9827da7572d68ea39817833f75b0ad8265f6bff9
Author: 吴晟 Wu Sheng <[email protected]>
AuthorDate: Mon Jul 6 18:06:41 2026 +0800
fix: support v1 trace query protocol for non-BanyanDB storages (#43)
---
.github/workflows/publish-docker.yaml | 6 +-
CLAUDE.md | 12 +++
internal/tools/trace.go | 112 ++++++++++++++++++++-
internal/tools/trace_test.go | 180 ++++++++++++++++++++++++++++++++++
4 files changed, 305 insertions(+), 5 deletions(-)
diff --git a/.github/workflows/publish-docker.yaml
b/.github/workflows/publish-docker.yaml
index 8fdb5d3..c7eab96 100644
--- a/.github/workflows/publish-docker.yaml
+++ b/.github/workflows/publish-docker.yaml
@@ -42,7 +42,7 @@ jobs:
steps:
- uses: actions/checkout@v4
- name: Set up Docker Buildx
- uses:
docker/setup-buildx-action@8d2750c68a42422c14e847fe6c8ac0403b4cbd6f
+ uses:
docker/setup-buildx-action@bb05f3f5519dd87d3ba754cc423b652a5edd6d2c # v4.2.0
- name: Resolve image version
run: |
if [ "${{ github.event_name }}" = "release" ]; then
@@ -53,14 +53,14 @@ jobs:
echo "VERSION=${VERSION}" >> "$GITHUB_ENV"
echo "Resolved VERSION=${VERSION}"
- name: Log in to GHCR
- uses: docker/login-action@c94ce9fb468520275223c153574b00df6fe4bcc9
+ uses: docker/login-action@af1e73f918a031802d376d3c8bbc3fe56130a9b0 #
v4.4.0
if: github.event_name == 'push' && github.ref == 'refs/heads/main'
with:
registry: ghcr.io
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}
- name: Log in to Docker Hub
- uses: docker/login-action@c94ce9fb468520275223c153574b00df6fe4bcc9
+ uses: docker/login-action@af1e73f918a031802d376d3c8bbc3fe56130a9b0 #
v4.4.0
if: github.event_name == 'release'
with:
registry: docker.io
diff --git a/CLAUDE.md b/CLAUDE.md
index 71b52de..ed610e0 100644
--- a/CLAUDE.md
+++ b/CLAUDE.md
@@ -112,6 +112,18 @@ All `.go` files must have the Apache 2.0 license header
(17-line block). Run `ma
### Error Handling in Tools
Tool handlers should return `(mcp.NewToolResultError(...), nil)` for expected
query failures (bad input, OAP errors), not `(nil, err)`. Reserve Go errors for
truly unexpected failures. Use the `ErrMarshalFailed` constant for JSON marshal
errors.
+### Comments
+Add a comment only when the code is not clear on its own — explain the
non-obvious *why* (a workaround, a compatibility constraint, a surprising side
effect), not the *what*. Do not write doc comments that merely restate a
function/const name. Example worth keeping: `queryTraceV1GQL` omits the
`duration` arg because it does not exist on OAP < 10.3.0.
+
## CI & Merge Policy
Squash-merge only. PRs to `main` require 1 approval and passing `Required`
status check (license + lint + docker build). Go 1.25.
+
+### GitHub Actions & the ASF allowed-actions list
+This repo is under `apache/*` and is governed by the ASF org-wide
allowed-actions policy, enforced on every workflow run:
+- Actions in `apache/*`, `actions/*`, and `github/*` are always allowed — no
pinning required.
+- Every **third-party** action (e.g. `docker/*`) must be pinned to a specific
commit **SHA** that is on the ASF approved list. The list **rotates**: old SHAs
are pruned as new action versions are approved, so a pin that passed months ago
can later be rejected with "action is not allowed". `publish-docker.yaml` only
runs on push-to-`main`/release, so a stale pin stays latent until the next
publish.
+- Before relying on or bumping a third-party action, check the SHA against the
approved list:
https://github.com/apache/infrastructure-actions/blob/main/approved_patterns.yml
(entries are `owner/action@<SHA>`; pick the newest approved SHA of that
action). Only if a needed action/SHA is absent does it require a PR to
`apache/infrastructure-actions`.
+
+### Commit authorship
+Commits produced with AI assistance credit the assistant as a co-author, e.g.
a `Co-Authored-By: Claude <...>` trailer, so the contribution is attributed
transparently.
diff --git a/internal/tools/trace.go b/internal/tools/trace.go
index 208a17c..785e65a 100644
--- a/internal/tools/trace.go
+++ b/internal/tools/trace.go
@@ -101,6 +101,107 @@ func tracesV2(ctx context.Context, condition
*api.TraceQueryCondition) (api.Trac
return response["result"], err
}
+const hasQueryTracesV2SupportGQL = `
+query {
+ result: hasQueryTracesV2Support
+}`
+
+const queryBasicTracesV1GQL = `
+query ($condition: TraceQueryCondition!) {
+ result: queryBasicTraces(condition: $condition) {
+ traces { segmentId endpointNames duration start isError
traceIds }
+ }
+}`
+
+// queryTraceV1GQL omits the queryTrace `duration` argument 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 }
+ }
+ }
+ }
+}`
+
+func queryTracesAuto(ctx context.Context, condition *api.TraceQueryCondition)
(api.TraceList, error) {
+ if supportsTraceV2(ctx) {
+ return tracesV2(ctx, condition)
+ }
+ return tracesV1(ctx, condition)
+}
+
+// supportsTraceV2 treats any error (field absent on an older OAP, or an
unreachable backend)
+// as "not supported" so the v1 path is used; a broken connection then
surfaces on that 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"]
+}
+
+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
+}
+
+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 fetches each unique trace's spans via queryTrace because
queryBasicTraces returns
+// summaries only. queryBasicTraces paginates over segments on non-BanyanDB
storage, so
+// page_size bounds segments (not traces) and segments of one trace are
de-duplicated here —
+// a broad query may return 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 +494,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 +711,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)
+ }
+}
