Add Traces toolset - #29
Merged
Merged
Conversation
saswatamcode
reviewed
Feb 6, 2026
saswatamcode
left a comment
saswatamcode
left a comment
Member
There was a problem hiding this comment.
Thanks a lot! Would love to see some evals for this :)
Some comments/questions in the meantime!
| "github.com/mark3labs/mcp-go/mcp" | ||
| ) | ||
|
|
||
| func ListInstancesTool() mcp.Tool { |
Member
There was a problem hiding this comment.
I'm probably missing something. How is the LLM going to decide which instance to pick?
Contributor
Author
There was a problem hiding this comment.
The LLM should ask the user.
Example from Claude Code:
❯ show me all traces with errors
● I'll help you find traces with errors. First, let me list the available Tempo instances.
● obs-mcp - tempo_list_instances (MCP)
⎿ {
"instances": [
{
… +21 lines (ctrl+o to expand)
● I found 2 Tempo instances available:
┌──────────┬─────────────────┬────────────────────┬────────┐
│ Instance │ Namespace │ Tenants │ Status │
├──────────┼─────────────────┼────────────────────┼────────┤
│ tempo1 │ obs-mcp-tracing │ project1, project2 │ Ready │
├──────────┼─────────────────┼────────────────────┼────────┤
│ tempo2 │ obs-mcp-tracing │ stage, prod │ Ready │
└──────────┴─────────────────┴────────────────────┴────────┘
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
☐ Instance
Which Tempo instance and tenant would you like to search for error traces?
❯ 1. tempo1 / project1
Query tempo1 instance with project1 tenant
2. tempo1 / project2
Query tempo1 instance with project2 tenant
3. tempo2 / stage
Query tempo2 instance with stage tenant
4. tempo2 / prod
Query tempo2 instance with prod tenant
5. Type something.
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
6. Chat about this
Member
There was a problem hiding this comment.
Oh! Well I'm not sure we have a selector UI for the Aladdin/OLS that this will eventually plug into
Contributor
Author
There was a problem hiding this comment.
That selector just popped up now (maybe it was a Claude Code update), previously it was a regular text question where I could reply with a text response.
| "github.com/mark3labs/mcp-go/mcp" | ||
| ) | ||
|
|
||
| func GetTraceByIdTool() mcp.Tool { |
Member
There was a problem hiding this comment.
How is the model getting traceID? Search traces tool?
Contributor
Author
There was a problem hiding this comment.
Yes, either the user knows the trace id and wants to query it directly, or the LLM should pick it up from the output of the search traces tool.
Member
There was a problem hiding this comment.
Ack! I think this will trip the model up a bit given amount of traces. (Need to figure out context mgmt for this data, pending problems on metrics side too)
| "github.com/mark3labs/mcp-go/mcp" | ||
| ) | ||
|
|
||
| func SearchTracesTool() mcp.Tool { |
Member
There was a problem hiding this comment.
Also, maybe we have some actual guardrails in place?
The goal of guardrails for MCPs like this would be to not only not make super wide queries on the o11y backend, but also to force it to make queries more accurate to lead the user to a correct answer
Contributor
Author
There was a problem hiding this comment.
I've added one guardrail now to the loader, to limit the limit parameter (limit parameter describes how many traces to return for a search query).
I think with time and experimentation more guardrails will come up in the future :)
|
|
||
| func NewMCPServer(opts ObsMCPOptions) (*server.MCPServer, error) { | ||
| hooks := &server.Hooks{} | ||
| hooks.AddBeforeCallTool(func(ctx context.Context, id any, message *mcp.CallToolRequest) { |
Member
There was a problem hiding this comment.
Oh TIL! That's actually really neat for some instrumentation!
slashpai
reviewed
Feb 6, 2026
slashpai
left a comment
slashpai
left a comment
Member
There was a problem hiding this comment.
A few nits, also unit tests and e2e tests for this would be great :)
I think you should be able to use manifests added in hack/tempo to setup, for prometheus and alertmanager we are using kube-prometheus not sure if something like that exists for tempo
Contributor
Author
|
Thank you both for the reviews! |
Contributor
Author
|
Updates:
|
Contributor
|
@andreasgerstmayr @saswatamcode I needed to hack the code for local development. If possible, adjust the PR to make it "local development friendly". You find what I hacked here falox@b087058 |
iNecas
reviewed
Apr 27, 2026
iNecas
reviewed
Apr 27, 2026
iNecas
reviewed
Apr 27, 2026
|
|
||
| func getHTTPClient(restConfig *rest.Config) (*http.Client, error) { | ||
| // Create HTTP client with Kubernetes authentication | ||
| rt, err := rest.TransportFor(restConfig) |
Contributor
There was a problem hiding this comment.
This seems to get a bit more elaborate, see https://github.com/rhobs/obs-mcp/blob/main/pkg/mcp/auth.go#L139, ideally the code could be shared between the toolsets?
Contributor
Author
There was a problem hiding this comment.
I re-used the auth code now. Ideally we'll unify mcp/auth.go and toolset/tools/prometheus_client.go, but that should be done outside this (already large) PR.
Contributor
|
Can we squash and rebase, and avoid merges from main? |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Tempo/traces toolset and five MCP tools ( Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (21)
evals/lightspeed/system.yaml-57-60 (1)
57-60:⚠️ Potential issue | 🟠 MajorAsk for
tempoNamespacetoo; instance + tenant is still underspecified.All Tempo query tools require
tempoNamespaceandtempoName. With the current wording, the assistant can still end up without enough inputs to call a Tempo tool, and it will also ask fortenantunnecessarily on single-tenant instances.Suggested fix
- Ask the user which Tempo instance and tenant to query if the user did not specify it explicitly. + Ask the user which Tempo namespace and instance (`tempoNamespace` and `tempoName`) to query if the user did not specify them explicitly. + Ask for `tenant` only when the selected Tempo instance is multi-tenant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@evals/lightspeed/system.yaml` around lines 57 - 60, Update the Tempo tools instructions to require both tempoNamespace and tempoName in addition to instance and (when applicable) tenant: ensure the text explicitly asks the user for tempoNamespace and tempoName if not provided, and change the tenant prompt logic so the assistant only asks for tenant when the target Tempo instance is multi-tenant; reference the existing Tempo tools wording ("Do not query across multiple instances..." and related lines) and add the tempoNamespace/tempoName requirement and conditional tenant prompt.pkg/tempo/client/client.go-37-50 (1)
37-50:⚠️ Potential issue | 🟠 MajorReject responses exceeding the size limit instead of silently truncating them.
Line 37 reads the response body with
io.LimitReader(resp.Body, maxResponseSize), butio.ReadAllreturns successfully when the body is larger than 10 MB—it simply stops reading at the limit and treats it as EOF. This causes the function to return truncated payloads as complete, valid responses, corrupting trace/search results.Suggested fix
- bodyBytes, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseSize)) + bodyBytes, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseSize+1)) if err != nil { return "", err } + if len(bodyBytes) > maxResponseSize { + return "", fmt.Errorf("response exceeded %d bytes", maxResponseSize) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tempo/client/client.go` around lines 37 - 50, The code reads the response via io.ReadAll(io.LimitReader(resp.Body, maxResponseSize)) which can silently truncate large responses; change the read logic so you detect truncation and reject oversized responses: read up to maxResponseSize into bodyBytes (keep using io.LimitReader or io.ReadFull), then if len(bodyBytes) == maxResponseSize attempt to read one more byte from resp.Body (e.g., read a single byte into a small buffer); if that extra read returns >0 (or io.EOF is not returned), return an error like "response too large" instead of returning truncated bodyBytes. Apply this around the existing use of maxResponseSize, resp.Body and variable bodyBytes in the function.pkg/tools/handlers.go-35-47 (1)
35-47:⚠️ Potential issue | 🟠 MajorValidate numeric inputs instead of silently truncating to
int.
GetIntconvertsfloat64tointwithout validation. This causes two issues:
- Fractional values are silently truncated (e.g.,
1.9becomes1).- Out-of-range
float64values can convert unpredictably.These parameters (
limit,maxStaleValues,spss) are integer-like tool inputs that should reject invalid data, not mutate it.Suggested fix
+import "math" + func GetInt(params map[string]any, key string, defaultValue int) int { if val, ok := params[key]; ok { switch v := val.(type) { case float64: - return int(v) + if v != math.Trunc(v) { + return defaultValue + } + i := int(v) + if float64(i) != v { + return defaultValue + } + return i case int: return v } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tools/handlers.go` around lines 35 - 47, GetInt currently truncates float64 to int; change it to validate numeric inputs instead: in GetInt, when params[key] is float64 (or json.Number), first verify the value is an exact integer (e.g., compare v to math.Trunc(v)), then verify it fits into Go's int range (compare against float64(maxInt) and float64(minInt)); only then convert and return int(v). If the value is fractional or out of range, do not truncate—return defaultValue (and optionally surface an error at the call site). Also handle other numeric types (int64, json.Number) explicitly and keep existing behavior for true ints; ensure callers for keys like "limit", "maxStaleValues", "spss" are unaffected by silent truncation.cmd/obs-mcp/main.go-100-100 (1)
100-100:⚠️ Potential issue | 🟠 MajorValidate and normalize
-toolsetsvalues instead of blind casting.Lines 144-151 accept raw CSV tokens without trimming or validation, so malformed input silently disables toolsets (for example,
metrics, traces).As per coding guidelines, "**: Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."Proposed fix
- opts := mcpserver.ObsMCPOptions{ - Toolsets: parseToolsets(*toolsets), + parsedToolsets, err := parseToolsets(*toolsets) + if err != nil { + log.Fatalf("Invalid toolsets: %v", err) + } + + opts := mcpserver.ObsMCPOptions{ + Toolsets: parsedToolsets, AuthMode: parsedAuthMode, @@ -func parseToolsets(toolsets string) []mcpserver.Toolset { +func parseToolsets(toolsets string) ([]mcpserver.Toolset, error) { parts := strings.Split(toolsets, ",") - result := make([]mcpserver.Toolset, len(parts)) - for i, p := range parts { - result[i] = mcpserver.Toolset(p) + result := make([]mcpserver.Toolset, 0, len(parts)) + seen := map[mcpserver.Toolset]struct{}{} + for _, raw := range parts { + p := strings.TrimSpace(raw) + if p == "" { + continue + } + ts := mcpserver.Toolset(p) + switch ts { + case mcpserver.ToolsetMetrics, mcpserver.ToolsetTraces: + if _, ok := seen[ts]; ok { + continue + } + seen[ts] = struct{}{} + result = append(result, ts) + default: + return nil, fmt.Errorf("unknown toolset %q (valid: %s,%s)", p, mcpserver.ToolsetMetrics, mcpserver.ToolsetTraces) + } } - return result + if len(result) == 0 { + return nil, fmt.Errorf("at least one toolset must be enabled") + } + return result, nil }Also applies to: 144-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/obs-mcp/main.go` at line 100, The parseToolsets call is currently taking raw CSV tokens without trimming or validating which allows malformed values like "metrics, traces" to silently disable toolsets; update the parseToolsets function to split the CSV, trim whitespace from each token, normalize (e.g., strings.ToLower) and validate each entry against an explicit allowed set (e.g., a map or slice of allowed toolset names), and make the caller handle invalid entries (either return an error or log a clear warning and skip/abort) so Toolsets: parseToolsets(*toolsets) only receives normalized, validated values.hack/e2e/manifests/tracing/04_testdata_k6.yaml-17-23 (1)
17-23:⚠️ Potential issue | 🟠 MajorHarden the Deployment security context.
Lines 17-23 run with default privileges; add explicit pod/container security settings (
runAsNonRoot,allowPrivilegeEscalation: false,readOnlyRootFilesystem: true, drop capabilities).As per coding guidelines, "**: Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."Proposed fix
spec: replicas: 1 selector: @@ template: metadata: labels: app.kubernetes.io/name: k6-tracing spec: + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault containers: - name: k6-tracing image: ghcr.io/grafana/xk6-client-tracing:v0.0.5 + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: ["ALL"] env: - name: ENDPOINT value: otel-collector.obs-mcp-tracing:4317🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e/manifests/tracing/04_testdata_k6.yaml` around lines 17 - 23, The deployment manifest runs the k6-tracing container with default privileges; update the PodSpec and container securityContext for the container named "k6-tracing" to harden runtime security: set pod-level securityContext.runAsNonRoot: true (and optionally runAsUser/FSGroup), and in the container securityContext set allowPrivilegeEscalation: false, readOnlyRootFilesystem: true, and drop all unnecessary capabilities (e.g., capabilities: drop: ["ALL"]). Apply these settings under the same spec where the "k6-tracing" container is declared so the manifest explicitly enforces non-root, no privilege escalation, read-only root filesystem, and capability dropping.pkg/tools/prompt.go-44-47 (1)
44-47:⚠️ Potential issue | 🟠 MajorClarify required Tempo identifiers to prevent avoidable tool failures.
Line 47 asks for “instance and tenant,” but Tempo tool handlers require both
tempoNamespaceandtempoName; tenant is only conditionally required for multi-tenant instances. This prompt can drive repeated invalid calls.Suggested prompt fix
-Ask the user which Tempo instance and tenant to query if the user did not specify it explicitly. +Ask the user which Tempo namespace and instance name to query if not specified explicitly. +If the selected Tempo instance is multi-tenant and tenant is not specified, ask for the tenant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tools/prompt.go` around lines 44 - 47, Update the Tempo tool prompt so it explicitly asks for tempoNamespace and tempoName (both required) and only asks for tenant when the user indicates or when querying a multi-tenant instance; replace the ambiguous “instance and tenant” phrasing with clear instructions that callers must supply tempoNamespace and tempoName and that tenant is conditional. Locate the prompt generation in pkg/tools/prompt.go (the Tempo tools prompt text) and revise the three lines beginning with “Do not query…” / “Ask the user…” to reflect these exact identifier names and the conditional tenant requirement so tool handlers receive valid inputs.hack/e2e/setup-cluster.sh-72-76 (1)
72-76:⚠️ Potential issue | 🟠 MajorFail fast when all tracing manifest apply retries are exhausted.
With
set -e, thekubectl apply ... && breakpattern in Line 73 won’t terminate the script if all retries fail; execution continues and fails later with less actionable errors.Suggested retry/fail-fast patch
-for i in $(seq 1 3); do - kubectl apply -f "${SCRIPT_DIR}/manifests/tracing" && break - echo " Retrying in 10s... (attempt $i/3)" - sleep 10 -done +applied=false +for i in $(seq 1 3); do + if kubectl apply -f "${SCRIPT_DIR}/manifests/tracing"; then + applied=true + break + fi + echo " Retrying in 10s... (attempt $i/3)" + sleep 10 +done +if [ "${applied}" != "true" ]; then + echo "ERROR: failed to apply tracing manifests after 3 attempts" >&2 + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e/setup-cluster.sh` around lines 72 - 76, The retry loop using "for i in $(seq 1 3); do" with "kubectl apply -f \"${SCRIPT_DIR}/manifests/tracing\" && break" can silently continue past failures under set -e; update the loop so it detects when all 3 attempts fail and exits non‑zero immediately. Implement a success check (e.g., a local success flag or check after the loop) or make the last attempt call exit 1 on failure; reference the for-loop and the kubectl apply command in hack/e2e/setup-cluster.sh to ensure the script fails fast if all retries are exhausted.tests/e2e/e2e_test.go-693-698 (1)
693-698:⚠️ Potential issue | 🟠 MajorMake instance assertions order-insensitive to avoid flaky E2E failures.
Line 695 asserts exact slice order. Discovery/list responses can reorder, so this can fail non-deterministically even when content is correct.
Suggested assertion change
- require.Equal(t, []any{ + require.ElementsMatch(t, []any{ map[string]any{"kind": "TempoStack", "tempoNamespace": "obs-mcp-tracing", "tempoName": "tempo1", "multitenancy": false, "status": "Ready"}, map[string]any{"kind": "TempoStack", "tempoNamespace": "obs-mcp-tracing", "tempoName": "tempo2", "multitenancy": false, "status": "Ready"}, }, instances)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/e2e_test.go` around lines 693 - 698, The test currently asserts slice order with require.Equal on instances extracted from structuredContent, which can be flaky; change the assertion to be order-insensitive by using require.ElementsMatch (or assert.ElementsMatch) to compare the expected slice to instances (or alternatively sort instances by "tempoName" before asserting). Update the assertion that references structured, instances and the require.Equal call so the test verifies membership regardless of element order.pkg/toolset/tools/prometheus_client.go-227-228 (1)
227-228:⚠️ Potential issue | 🟠 MajorSet an HTTP timeout on the Tempo client.
Line 227 creates an
http.ClientwithoutTimeout; slow/broken upstreams can block tool calls indefinitely.Suggested fix
import ( @@ "net/http" "os" "strings" + "time" @@ - httpClient := &http.Client{Transport: apiConfig.RoundTripper} + httpClient := &http.Client{ + Transport: apiConfig.RoundTripper, + Timeout: 30 * time.Second, + } return tempoclient.NewTempoLoader(httpClient, url), nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/toolset/tools/prometheus_client.go` around lines 227 - 228, The http.Client created in prometheus_client.go (variable httpClient) is missing a Timeout which can cause blocking; modify the construction of httpClient to include a sensible Timeout (e.g., 15–30s or derive from existing config) while keeping apiConfig.RoundTripper, then pass that timed-out client into tempoclient.NewTempoLoader(url) so the Tempo loader cannot hang indefinitely; update any related configuration or tests if they expect no timeout.hack/tempo_multitenancy_openshift/06_testdata_hotrod.yaml-16-39 (1)
16-39:⚠️ Potential issue | 🟠 MajorHarden pod/container security context for the test workload.
Line 16-39 uses default security context (root-capable, privilege escalation allowed, writable root FS). Even in test manifests, this weakens cluster security posture.
Suggested hardening patch
spec: + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault selector: @@ spec: containers: - image: jaegertracing/example-hotrod:1.46 name: hotrod + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: ["ALL"] args: @@ resources: @@ + volumeMounts: + - name: tmp + mountPath: /tmp + volumes: + - name: tmp + emptyDir: {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/tempo_multitenancy_openshift/06_testdata_hotrod.yaml` around lines 16 - 39, The hotrod container spec currently has no securityContext and allows root-capable behavior; add a hardened securityContext on the container (or pod) for the hotrod container to enforce non-root execution and prevent privilege escalation: set runAsNonRoot: true and a non-root runAsUser (e.g., 1000), set allowPrivilegeEscalation: false, drop all capabilities (capabilities.drop: ["ALL"]), enable readOnlyRootFilesystem: true, and set a seccompProfile (type: RuntimeDefault) — update the container named "hotrod" in this manifest to include these fields so the test workload is not root-capable, cannot escalate privileges, and uses a read-only root filesystem.hack/tempo_multitenancy_openshift/02_minio.yaml-30-53 (1)
30-53:⚠️ Potential issue | 🟠 MajorMinIO deployment should not run with default security privileges.
The pod/container security context is not set, so it inherits permissive defaults (root-capable profile, privilege escalation path).
Suggested hardening
spec: template: spec: + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault containers: - name: minio + securityContext: + allowPrivilegeEscalation: false + capabilities: + drop: + - ALL + # Enable if workload supports it: + # readOnlyRootFilesystem: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/tempo_multitenancy_openshift/02_minio.yaml` around lines 30 - 53, The MinIO container "minio" is missing a securityContext and thus runs with permissive defaults; add a pod- or container-level securityContext for the "minio" container to harden it: set runAsNonRoot: true and runAsUser to a non-root UID (e.g., 1000), set allowPrivilegeEscalation: false, drop all Linux capabilities (capabilities.drop: ["ALL"]), enable readOnlyRootFilesystem: true where feasible, and apply a seccompProfile (runtime/default) and fsGroup/gid as needed for the /storage PVC; update the spec under the container named "minio" (or top-level pod securityContext) to include these fields so the MinIO process cannot run as root or escalate privileges.hack/tempo_multitenancy_openshift/05_testdata_k6.yaml-17-23 (1)
17-23:⚠️ Potential issue | 🟠 MajorHarden pod/container security context for the k6 workload.
The deployment currently runs with default privileges (root-capable, privilege escalation allowed by default), which is an avoidable security risk.
Suggested hardening
spec: template: spec: + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault containers: - name: k6-tracing image: ghcr.io/grafana/xk6-client-tracing:v0.0.5 + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: + - ALL🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/tempo_multitenancy_openshift/05_testdata_k6.yaml` around lines 17 - 23, The k6 container "k6-tracing" is running with default root/privileged settings; update the Pod/Container securityContext for the "k6-tracing" container to harden it: set runAsNonRoot: true and a non-zero runAsUser (e.g., 1000), set allowPrivilegeEscalation: false, drop all Linux capabilities, enable readOnlyRootFilesystem: true, and add a seccompProfile (runtime/default) and fsGroup at the pod spec if needed; apply these changes to the container spec that contains name: k6-tracing and env: ENDPOINT to ensure the workload runs unprivileged and with a locked-down filesystem.hack/e2e/manifests/tracing/01_minio.yaml-30-53 (1)
30-53:⚠️ Potential issue | 🟠 MajorE2E MinIO pod should not rely on default security context.
This deployment currently runs without explicit pod/container hardening, which leaves unnecessary privilege surface in the test cluster.
Suggested hardening
spec: template: spec: + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault containers: - name: minio + securityContext: + allowPrivilegeEscalation: false + capabilities: + drop: + - ALL + # Enable if workload supports it: + # readOnlyRootFilesystem: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e/manifests/tracing/01_minio.yaml` around lines 30 - 53, Add explicit pod- and container-level securityContext to harden the MinIO pod: set a pod securityContext with runAsNonRoot: true, runAsUser: 1000 and fsGroup: 1000 so the /storage PVC is owned by a non-root user, and on the minio container (container name "minio") add a securityContext with allowPrivilegeEscalation: false, privileged: false, capabilities: { drop: ["ALL"] }, and seccompProfile: { type: "RuntimeDefault" }; keep the /storage volumeMount writable for MinIO (do not set readOnlyRootFilesystem) and ensure these fields are added under the existing spec -> containers and spec top-level securityContext blocks.hack/tempo_multitenancy_openshift/04_tempo.yaml-41-53 (1)
41-53:⚠️ Potential issue | 🟠 MajorScope trace-read RBAC tighter than
system:authenticatedThis binding grants
project1trace-read access to every authenticated cluster user, which is overly permissive for trace data. Bind this role to a dedicated service account or a narrowly scoped group used by the MCP workflow instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/tempo_multitenancy_openshift/04_tempo.yaml` around lines 41 - 53, The ClusterRoleBinding obs-mcp-tracing-traces-reader-project1 currently binds the ClusterRole obs-mcp-tracing-traces-reader-project1 to the broad Group subject system:authenticated; tighten this by replacing that subject with a dedicated ServiceAccount (or a more specific group) used by the MCP workflow (e.g., set kind: ServiceAccount and name: <mcp-service-account> and namespace: <mcp-namespace>) or an explicit narrow group; ensure the subject block is updated accordingly and re-apply so only the intended MCP principal has trace-read access.pkg/tempo/search_traces.go-102-120 (1)
102-120:⚠️ Potential issue | 🟠 MajorReject invalid time windows (
startafterend)The handler forwards time bounds without validating ordering. Add an explicit check so invalid ranges fail fast with a clear message instead of relying on downstream Tempo behavior.
Proposed fix
start, err := parseDate(tools.GetString(args, "start", "")) if err != nil { return SearchTracesOutput{}, fmt.Errorf("invalid start time: %v", err) } end, err := parseDate(tools.GetString(args, "end", "")) if err != nil { return SearchTracesOutput{}, fmt.Errorf("invalid end time: %v", err) } + if start != 0 && end != 0 && start > end { + return SearchTracesOutput{}, fmt.Errorf("invalid time range: start must be before end") + } limit := tools.GetInt(args, "limit", 0) spss := tools.GetInt(args, "spss", 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tempo/search_traces.go` around lines 102 - 120, After parsing start and end (variables start and end) but before creating tempoclient.SearchOptions, add an explicit validation that start is not after end (e.g. if start.After(end) { return SearchTracesOutput{}, fmt.Errorf("invalid time window: start must be <= end") }) so the handler fails fast with a clear message; place this check between the parseDate calls and the construction of opts to prevent forwarding invalid ranges downstream.pkg/tempo/search_tag_values.go-79-99 (1)
79-99:⚠️ Potential issue | 🟠 MajorValidate time range and numeric parameters before calling Tempo
The handler accepts invalid ranges (
start > end) and negative numeric values forlimit/maxStaleValues. Fail fast here to avoid avoidable backend errors and expensive/undefined query behavior.Proposed fix
start, err := parseDate(tools.GetString(args, "start", "")) if err != nil { return SearchTagValuesOutput{}, fmt.Errorf("invalid start time: %v", err) } end, err := parseDate(tools.GetString(args, "end", "")) if err != nil { return SearchTagValuesOutput{}, fmt.Errorf("invalid end time: %v", err) } + if start != 0 && end != 0 && start > end { + return SearchTagValuesOutput{}, fmt.Errorf("invalid time range: start must be before end") + } query := tools.GetString(args, "query", "") limit := tools.GetInt(args, "limit", 0) maxStaleValues := tools.GetInt(args, "maxStaleValues", 0) + if limit < 0 { + return SearchTagValuesOutput{}, fmt.Errorf("limit parameter must be >= 0") + } + if maxStaleValues < 0 { + return SearchTagValuesOutput{}, fmt.Errorf("maxStaleValues parameter must be >= 0") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tempo/search_tag_values.go` around lines 79 - 99, Validate the parsed times and numeric params before building tempoclient.SearchTagValuesV2Options: after obtaining start and end from parseDate and limit/maxStaleValues from tools.GetInt, check that start <= end (return an error like "start must be <= end") and that limit and maxStaleValues are non-negative (return errors like "limit must be >= 0"); apply these checks where start, end, limit, and maxStaleValues are set so you fail fast instead of calling SearchTagValuesV2Options with invalid values.pkg/mcp/server.go-112-118 (1)
112-118:⚠️ Potential issue | 🟠 MajorDon’t make trace tool registration depend on a local kubeconfig.
k8s.GetClientConfig()currently only loads kubeconfig, so enablingToolsetTracesmakes server startup fail anywhere that file is absent, including in-cluster deployments. Either fall back to in-cluster config or create the dynamic client lazily inside the Tempo handlers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/mcp/server.go` around lines 112 - 118, The current startup path calls k8s.GetClientConfig() and dynamic.NewForConfig() unconditionally when ToolsetTraces is enabled, which fails if only in-cluster config exists; update the code so trace tool registration does not depend on a local kubeconfig: either attempt GetClientConfig() and on error fall back to in-cluster config (e.g., use rest.InClusterConfig()) before calling dynamic.NewForConfig(), or remove eager client creation and construct the dynamic client lazily inside the Tempo trace handlers (create the client on first request and cache it); reference k8s.GetClientConfig(), dynamic.NewForConfig(), ToolsetTraces, and the Tempo handler initialization when applying the change.pkg/mcp/server.go-67-79 (1)
67-79:⚠️ Potential issue | 🟠 MajorAvoid logging raw tool arguments.
These values can contain trace IDs, tenant names, and user-supplied query filters. Emitting them verbatim to debug logs creates an avoidable data-retention risk; log only the tool name and argument keys, or redact values first.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/mcp/server.go` around lines 67 - 79, The middleware currently unmarshals CallToolParamsRaw arguments and logs each key/value pair via slog.Any (in the mcpServer.AddReceivingMiddleware anonymous MethodHandler), which exposes sensitive user/tenant/trace data; change the logging to only include the tool name and argument keys or redact values. Specifically, in the CallToolParamsRaw case (where req.GetParams() is type *mcp.CallToolParamsRaw) stop appending vals with slog.Any(k, v) — instead collect only the argument keys (e.g., build a []string of map keys or append slog.String(key, "<redacted>") for each key) and log those keys or redacted placeholders in the slog.Debug call so no raw values are emitted. Ensure the rest of the middleware and error handling remains unchanged.pkg/mcp/server.go-107-124 (1)
107-124:⚠️ Potential issue | 🟠 MajorFail fast when
ObsMCPOptions.Tempois nil.The Tempo handlers receive
opts.Tempodirectly, andpkg/tempo/common.godereferencesparams.config.UseRoute. If traces are enabled without initializing this pointer, the first Tempo call will panic.Suggested guard
if slices.Contains(opts.Toolsets, ToolsetTraces) { + if opts.Tempo == nil { + return errors.New("tempo configuration must be provided when traces toolset is enabled") + } tempoToolset := &tempo.Toolset{} newTempoClient := func(ctx context.Context, url string) (tempoclient.Loader, error) { return getTempoHTTPClient(ctx, opts, url) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/mcp/server.go` around lines 107 - 124, Guard against a nil Tempo config before registering Tempo tools: check that opts.Tempo (from the surrounding ObsMCPOptions) is non-nil when slices.Contains(opts.Toolsets, ToolsetTraces) is true and return an error immediately if it is nil; this prevents nil dereference in tempo.ToMCPHandler / tempoToolset.*Handler (which read params.config.UseRoute). Ensure the check is placed before constructing tempoToolset, newTempoClient, or calling tempo.ToMCPHandler so registration stops early with a clear error.pkg/tempo/discovery/discovery.go-62-73 (1)
62-73:⚠️ Potential issue | 🟠 MajorBase multitenancy on the CR config, not on
Authenticationbeing non-empty.
Authenticationis tenant metadata, not the multitenancy switch. With the current checks, a multitenant instance with externally managed auth/no listed tenants is treated as single-tenant, which also changes the discovered service (gatewayvsquery-frontend/monolith) and makesGetURL()omit the tenant path. Please derive the boolean from the actual multitenancy config (spec.multitenancy.enabledforTempoMonolithic, and the TempoStack tenants config/mode separately) and populateTenantsonly when auth entries exist.Also applies to: 110-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tempo/discovery/discovery.go` around lines 62 - 73, The multitenancy boolean should be derived from the CR's explicit multitenancy configuration instead of testing for tempo.Spec.Tenants.Authentication being non-empty; update the logic that sets multitenancy (and the related serviceName/tenants assignment in the block that currently references multitenancy, DNSName, serviceName, and tenants) to check spec.multitenancy.enabled for TempoMonolithic and the TempoStack tenants/mode field for stack-based CRs, and only populate the tenants slice from tempo.Spec.Tenants.Authentication when auth entries actually exist; mirror the same change for the similar logic around lines 110-121 so GetURL() and service selection use the proper multitenancy flag and tenant path behavior.pkg/tempo/discovery/discovery.go-55-78 (1)
55-78:⚠️ Potential issue | 🟠 MajorDon't fail the entire discovery on one broken Tempo resource.
Any unparseable CR or unresolved route currently aborts
listTempoStacks/listTempoMonolithics, which makesListInstances()fail completely. Since discovery is used by the Tempo tools to find/validate instances, one unrelated broken object can take all Tempo operations down. Prefer skipping the bad instance and surfacing per-instance errors separately, or aggregate errors after collecting the healthy results.Also applies to: 103-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tempo/discovery/discovery.go` around lines 55 - 78, The loop in listTempoStacks currently returns immediately on parse or resolveBaseURL errors (runtime.DefaultUnstructuredConverter.FromUnstructured and resolveBaseURL), causing ListInstances to fail entirely; instead, skip the bad Tempo resource and continue collecting healthy instances while appending per-instance errors to an error aggregator (or slice) to return alongside results. Modify the loop over list.Items in listTempoStacks (and the similar block in listTempoMonolithics) to: on FromUnstructured errors or resolveBaseURL failures, record a contextual error (including tempo.Name/tempo.Namespace and the underlying error) into an errors slice and continue; only return a combined error (or nil) after iterating all items, and ensure the function still returns all successfully resolved baseURLs/instances. Ensure DNSName, TempoStack, multitenancy logic and tenants accumulation remain unchanged while skipping problematic entries.
🟡 Minor comments (3)
.gitignore-21-22 (1)
21-22:⚠️ Potential issue | 🟡 MinorConsider tightening/confirming
.claudeignore pattern (file vs directory).You added:
- Line 21:
/vendor- Line 22:
/.claudeIf
.claudeis a directory (common), consider ignoring it with a trailing slash (e.g./.claude/) to avoid missing nested contents. If it’s definitely a file, your current pattern is fine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 21 - 22, The .gitignore entry for ".claude" is ambiguous about whether it is a file or directory; if ".claude" is a directory (common), update the pattern to explicitly ignore the directory by changing the entry to use a trailing slash (e.g., "/.claude/") so nested contents are ignored, otherwise keep the existing "/.claude" if it is definitely a single file; locate the ".claude" line in the .gitignore and adjust the pattern accordingly.hack/tempo_multitenancy_openshift/README.md-10-17 (1)
10-17:⚠️ Potential issue | 🟡 MinorAdd a language identifier to the fenced code block.
Line 10 uses an unlabeled code fence, which fails markdownlint (
MD040).Suggested doc fix
-``` +```bash kubectl create serviceaccount demo TOKEN=$(kubectl create token demo) curl -G -k \ @@ -``` +```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/tempo_multitenancy_openshift/README.md` around lines 10 - 17, The fenced code block containing the shell commands (kubectl create serviceaccount demo, TOKEN=$(kubectl create token demo), curl -G -k ...) is missing a language identifier; update the opening fence to include "bash" (i.e., change the triple backtick that begins the block to ```bash) so markdownlint rule MD040 is satisfied and the block is correctly highlighted.TOOLS.md-287-397 (1)
287-397:⚠️ Potential issue | 🟡 MinorFix malformed Markdown table rows in generated Tempo docs
Several parameter descriptions are split across lines in table rows, which breaks column structure and triggers markdownlint MD055/MD056. This will render incorrectly in Markdown viewers and can fail docs linting. Since this file is generated, the fix should be in the generator/template so each table row remains structurally valid (e.g., single-line cell or explicit
<br>within one cell).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TOOLS.md` around lines 287 - 397, The generated Markdown tables have cells broken across lines (e.g., parameter rows for end, start, tenant, and descriptions under tempo_search_traces/tempo_search_tags/tempo_search_tag_values), which breaks table structure; update the docs generator/template that emits these table rows to collapse or replace internal newlines in cell content so each table cell is emitted as a single-line cell (or explicitly use HTML <br> for intended line breaks), e.g., sanitize the description strings for parameters like "end", "start", "limit", "spss", "query" before rendering the table and ensure the template joins multi-line descriptions into one logical cell to preserve valid Markdown table rows.
🧹 Nitpick comments (4)
pkg/mcp/tools.go (1)
10-13: Update theAllToolscomment now that Tempo tools live outsidepkg/tools.Line 13 merges registries from both
pkg/toolsandpkg/tempo, but the comment above still points future additions only topkg/tools/definitions.go.Suggested fix
-// AllTools returns all available MCP tools. -// When adding a new tool, add it to pkg/tools/definitions.go to keep both MCP and Toolset in sync, as well as docs. +// AllTools returns all available MCP tools. +// When adding a new tool, keep the owning package registry (`pkg/tools` or `pkg/tempo`) and the docs in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/mcp/tools.go` around lines 10 - 13, The comment for AllTools is outdated: update the function doc above AllTools() to mention that tools are now merged from both tools.AllTools() and tempo.AllTools(), and instruct contributors to add new tool definitions in the correct registries (e.g., pkg/tools/definitions.go for core tools and the tempo package's definitions file for tempo tools) so both MCP and the Toolset remain in sync; reference the AllTools function and the calls tools.AllTools() and tempo.AllTools() when making this change.hack/e2e/manifests/tracing/01_minio.yaml (1)
43-43: Use a pinned MinIO image in e2e manifests.A floating image (
docker.io/minio/minio) can change silently and make e2e results drift over time.Suggested change
- image: docker.io/minio/minio + image: docker.io/minio/minio@sha256:<tested-digest>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e/manifests/tracing/01_minio.yaml` at line 43, The manifest uses an unpinned floating MinIO image string "image: docker.io/minio/minio"; replace it with a pinned image tag or digest (for example "image: docker.io/minio/minio:<specific-tag>" or a sha256 digest) to prevent silent drift in e2e runs, updating the "image: docker.io/minio/minio" entry in the 01_minio manifest to a concrete tag or digest and ensure the chosen tag is recorded in CI/documentation.hack/tempo_multitenancy_openshift/02_minio.yaml (1)
43-43: Pin the MinIO image to a tested tag or digest.
docker.io/minio/miniois floating and can change between runs, which weakens reproducibility and supply-chain control.Suggested change
- image: docker.io/minio/minio + image: docker.io/minio/minio@sha256:<tested-digest>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/tempo_multitenancy_openshift/02_minio.yaml` at line 43, Replace the floating image reference "image: docker.io/minio/minio" with a pinned tag or digest (e.g., docker.io/minio/minio:<tested-tag> or docker.io/minio/minio@sha256:<digest>) in the container image field so the manifest uses a reproducible, tested MinIO release; update the Deploy/StatefulSet spec where "image" is set and document the chosen tag/digest so future updates go through an explicit review process or automated dependency updates.hack/tempo_multitenancy_openshift/00_operators.yaml (1)
20-24: Operator subscriptions are non-deterministic with automatic approvals.Using
channel: stable+installPlanApproval: Automaticcan upgrade to new operator bundles without review, which makes local/dev reproducibility weaker and can introduce unexpected regressions.Suggested change
spec: channel: stable - installPlanApproval: Automatic + installPlanApproval: Manual + # Optional but recommended for reproducibility: + # startingCSV: <tested-csv-version>Apply the same change to both subscriptions.
Also applies to: 45-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/tempo_multitenancy_openshift/00_operators.yaml` around lines 20 - 24, Subscription entries currently use channel: stable and installPlanApproval: Automatic which allows non-deterministic, unreviewed operator upgrades; update both subscription blocks by replacing channel: stable with a pinned/fixed channel name (not "stable") and set installPlanApproval: Manual so upgrades require an install plan approval/review; apply this change to the subscription for opentelemetry-product and the other subscription block referenced in the diff.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/mcp/auth.go`:
- Around line 125-126: The http.Client created for Tempo (httpClient :=
&http.Client{Transport: apiConfig.RoundTripper}) has no Timeout and can hang;
add a Timeout value to the client (e.g., Timeout: 10*time.Second or a
configurable value from apiConfig) before calling
tempoclient.NewTempoLoader(url) so requests cannot block indefinitely, updating
the httpClient construction to include the Timeout field.
In `@pkg/tempo/discovery/sanitize.go`:
- Around line 24-29: The loop uses byte offsets from "for i, x := range
strings.ToLower(name)" but compares i to utf8.RuneCountInString(name)-1, causing
incorrect boundary checks for multibyte runes; change the loop to iterate over
runes (e.g., convert to runes: runes := []rune(strings.ToLower(name)) and use
"for i, r := range runes") so the index i is a rune index, update
regex.MatchString(string(r)) and append r to d (or 'a' for boundaries)
accordingly, and compute the last index using len(runes)-1 to fix the boundary
detection.
---
Major comments:
In `@cmd/obs-mcp/main.go`:
- Line 100: The parseToolsets call is currently taking raw CSV tokens without
trimming or validating which allows malformed values like "metrics, traces" to
silently disable toolsets; update the parseToolsets function to split the CSV,
trim whitespace from each token, normalize (e.g., strings.ToLower) and validate
each entry against an explicit allowed set (e.g., a map or slice of allowed
toolset names), and make the caller handle invalid entries (either return an
error or log a clear warning and skip/abort) so Toolsets:
parseToolsets(*toolsets) only receives normalized, validated values.
In `@evals/lightspeed/system.yaml`:
- Around line 57-60: Update the Tempo tools instructions to require both
tempoNamespace and tempoName in addition to instance and (when applicable)
tenant: ensure the text explicitly asks the user for tempoNamespace and
tempoName if not provided, and change the tenant prompt logic so the assistant
only asks for tenant when the target Tempo instance is multi-tenant; reference
the existing Tempo tools wording ("Do not query across multiple instances..."
and related lines) and add the tempoNamespace/tempoName requirement and
conditional tenant prompt.
In `@hack/e2e/manifests/tracing/01_minio.yaml`:
- Around line 30-53: Add explicit pod- and container-level securityContext to
harden the MinIO pod: set a pod securityContext with runAsNonRoot: true,
runAsUser: 1000 and fsGroup: 1000 so the /storage PVC is owned by a non-root
user, and on the minio container (container name "minio") add a securityContext
with allowPrivilegeEscalation: false, privileged: false, capabilities: { drop:
["ALL"] }, and seccompProfile: { type: "RuntimeDefault" }; keep the /storage
volumeMount writable for MinIO (do not set readOnlyRootFilesystem) and ensure
these fields are added under the existing spec -> containers and spec top-level
securityContext blocks.
In `@hack/e2e/manifests/tracing/04_testdata_k6.yaml`:
- Around line 17-23: The deployment manifest runs the k6-tracing container with
default privileges; update the PodSpec and container securityContext for the
container named "k6-tracing" to harden runtime security: set pod-level
securityContext.runAsNonRoot: true (and optionally runAsUser/FSGroup), and in
the container securityContext set allowPrivilegeEscalation: false,
readOnlyRootFilesystem: true, and drop all unnecessary capabilities (e.g.,
capabilities: drop: ["ALL"]). Apply these settings under the same spec where the
"k6-tracing" container is declared so the manifest explicitly enforces non-root,
no privilege escalation, read-only root filesystem, and capability dropping.
In `@hack/e2e/setup-cluster.sh`:
- Around line 72-76: The retry loop using "for i in $(seq 1 3); do" with
"kubectl apply -f \"${SCRIPT_DIR}/manifests/tracing\" && break" can silently
continue past failures under set -e; update the loop so it detects when all 3
attempts fail and exits non‑zero immediately. Implement a success check (e.g., a
local success flag or check after the loop) or make the last attempt call exit 1
on failure; reference the for-loop and the kubectl apply command in
hack/e2e/setup-cluster.sh to ensure the script fails fast if all retries are
exhausted.
In `@hack/tempo_multitenancy_openshift/02_minio.yaml`:
- Around line 30-53: The MinIO container "minio" is missing a securityContext
and thus runs with permissive defaults; add a pod- or container-level
securityContext for the "minio" container to harden it: set runAsNonRoot: true
and runAsUser to a non-root UID (e.g., 1000), set allowPrivilegeEscalation:
false, drop all Linux capabilities (capabilities.drop: ["ALL"]), enable
readOnlyRootFilesystem: true where feasible, and apply a seccompProfile
(runtime/default) and fsGroup/gid as needed for the /storage PVC; update the
spec under the container named "minio" (or top-level pod securityContext) to
include these fields so the MinIO process cannot run as root or escalate
privileges.
In `@hack/tempo_multitenancy_openshift/04_tempo.yaml`:
- Around line 41-53: The ClusterRoleBinding
obs-mcp-tracing-traces-reader-project1 currently binds the ClusterRole
obs-mcp-tracing-traces-reader-project1 to the broad Group subject
system:authenticated; tighten this by replacing that subject with a dedicated
ServiceAccount (or a more specific group) used by the MCP workflow (e.g., set
kind: ServiceAccount and name: <mcp-service-account> and namespace:
<mcp-namespace>) or an explicit narrow group; ensure the subject block is
updated accordingly and re-apply so only the intended MCP principal has
trace-read access.
In `@hack/tempo_multitenancy_openshift/05_testdata_k6.yaml`:
- Around line 17-23: The k6 container "k6-tracing" is running with default
root/privileged settings; update the Pod/Container securityContext for the
"k6-tracing" container to harden it: set runAsNonRoot: true and a non-zero
runAsUser (e.g., 1000), set allowPrivilegeEscalation: false, drop all Linux
capabilities, enable readOnlyRootFilesystem: true, and add a seccompProfile
(runtime/default) and fsGroup at the pod spec if needed; apply these changes to
the container spec that contains name: k6-tracing and env: ENDPOINT to ensure
the workload runs unprivileged and with a locked-down filesystem.
In `@hack/tempo_multitenancy_openshift/06_testdata_hotrod.yaml`:
- Around line 16-39: The hotrod container spec currently has no securityContext
and allows root-capable behavior; add a hardened securityContext on the
container (or pod) for the hotrod container to enforce non-root execution and
prevent privilege escalation: set runAsNonRoot: true and a non-root runAsUser
(e.g., 1000), set allowPrivilegeEscalation: false, drop all capabilities
(capabilities.drop: ["ALL"]), enable readOnlyRootFilesystem: true, and set a
seccompProfile (type: RuntimeDefault) — update the container named "hotrod" in
this manifest to include these fields so the test workload is not root-capable,
cannot escalate privileges, and uses a read-only root filesystem.
In `@pkg/mcp/server.go`:
- Around line 112-118: The current startup path calls k8s.GetClientConfig() and
dynamic.NewForConfig() unconditionally when ToolsetTraces is enabled, which
fails if only in-cluster config exists; update the code so trace tool
registration does not depend on a local kubeconfig: either attempt
GetClientConfig() and on error fall back to in-cluster config (e.g., use
rest.InClusterConfig()) before calling dynamic.NewForConfig(), or remove eager
client creation and construct the dynamic client lazily inside the Tempo trace
handlers (create the client on first request and cache it); reference
k8s.GetClientConfig(), dynamic.NewForConfig(), ToolsetTraces, and the Tempo
handler initialization when applying the change.
- Around line 67-79: The middleware currently unmarshals CallToolParamsRaw
arguments and logs each key/value pair via slog.Any (in the
mcpServer.AddReceivingMiddleware anonymous MethodHandler), which exposes
sensitive user/tenant/trace data; change the logging to only include the tool
name and argument keys or redact values. Specifically, in the CallToolParamsRaw
case (where req.GetParams() is type *mcp.CallToolParamsRaw) stop appending vals
with slog.Any(k, v) — instead collect only the argument keys (e.g., build a
[]string of map keys or append slog.String(key, "<redacted>") for each key) and
log those keys or redacted placeholders in the slog.Debug call so no raw values
are emitted. Ensure the rest of the middleware and error handling remains
unchanged.
- Around line 107-124: Guard against a nil Tempo config before registering Tempo
tools: check that opts.Tempo (from the surrounding ObsMCPOptions) is non-nil
when slices.Contains(opts.Toolsets, ToolsetTraces) is true and return an error
immediately if it is nil; this prevents nil dereference in tempo.ToMCPHandler /
tempoToolset.*Handler (which read params.config.UseRoute). Ensure the check is
placed before constructing tempoToolset, newTempoClient, or calling
tempo.ToMCPHandler so registration stops early with a clear error.
In `@pkg/tempo/client/client.go`:
- Around line 37-50: The code reads the response via
io.ReadAll(io.LimitReader(resp.Body, maxResponseSize)) which can silently
truncate large responses; change the read logic so you detect truncation and
reject oversized responses: read up to maxResponseSize into bodyBytes (keep
using io.LimitReader or io.ReadFull), then if len(bodyBytes) == maxResponseSize
attempt to read one more byte from resp.Body (e.g., read a single byte into a
small buffer); if that extra read returns >0 (or io.EOF is not returned), return
an error like "response too large" instead of returning truncated bodyBytes.
Apply this around the existing use of maxResponseSize, resp.Body and variable
bodyBytes in the function.
In `@pkg/tempo/discovery/discovery.go`:
- Around line 62-73: The multitenancy boolean should be derived from the CR's
explicit multitenancy configuration instead of testing for
tempo.Spec.Tenants.Authentication being non-empty; update the logic that sets
multitenancy (and the related serviceName/tenants assignment in the block that
currently references multitenancy, DNSName, serviceName, and tenants) to check
spec.multitenancy.enabled for TempoMonolithic and the TempoStack tenants/mode
field for stack-based CRs, and only populate the tenants slice from
tempo.Spec.Tenants.Authentication when auth entries actually exist; mirror the
same change for the similar logic around lines 110-121 so GetURL() and service
selection use the proper multitenancy flag and tenant path behavior.
- Around line 55-78: The loop in listTempoStacks currently returns immediately
on parse or resolveBaseURL errors
(runtime.DefaultUnstructuredConverter.FromUnstructured and resolveBaseURL),
causing ListInstances to fail entirely; instead, skip the bad Tempo resource and
continue collecting healthy instances while appending per-instance errors to an
error aggregator (or slice) to return alongside results. Modify the loop over
list.Items in listTempoStacks (and the similar block in listTempoMonolithics)
to: on FromUnstructured errors or resolveBaseURL failures, record a contextual
error (including tempo.Name/tempo.Namespace and the underlying error) into an
errors slice and continue; only return a combined error (or nil) after iterating
all items, and ensure the function still returns all successfully resolved
baseURLs/instances. Ensure DNSName, TempoStack, multitenancy logic and tenants
accumulation remain unchanged while skipping problematic entries.
In `@pkg/tempo/search_tag_values.go`:
- Around line 79-99: Validate the parsed times and numeric params before
building tempoclient.SearchTagValuesV2Options: after obtaining start and end
from parseDate and limit/maxStaleValues from tools.GetInt, check that start <=
end (return an error like "start must be <= end") and that limit and
maxStaleValues are non-negative (return errors like "limit must be >= 0"); apply
these checks where start, end, limit, and maxStaleValues are set so you fail
fast instead of calling SearchTagValuesV2Options with invalid values.
In `@pkg/tempo/search_traces.go`:
- Around line 102-120: After parsing start and end (variables start and end) but
before creating tempoclient.SearchOptions, add an explicit validation that start
is not after end (e.g. if start.After(end) { return SearchTracesOutput{},
fmt.Errorf("invalid time window: start must be <= end") }) so the handler fails
fast with a clear message; place this check between the parseDate calls and the
construction of opts to prevent forwarding invalid ranges downstream.
In `@pkg/tools/handlers.go`:
- Around line 35-47: GetInt currently truncates float64 to int; change it to
validate numeric inputs instead: in GetInt, when params[key] is float64 (or
json.Number), first verify the value is an exact integer (e.g., compare v to
math.Trunc(v)), then verify it fits into Go's int range (compare against
float64(maxInt) and float64(minInt)); only then convert and return int(v). If
the value is fractional or out of range, do not truncate—return defaultValue
(and optionally surface an error at the call site). Also handle other numeric
types (int64, json.Number) explicitly and keep existing behavior for true ints;
ensure callers for keys like "limit", "maxStaleValues", "spss" are unaffected by
silent truncation.
In `@pkg/tools/prompt.go`:
- Around line 44-47: Update the Tempo tool prompt so it explicitly asks for
tempoNamespace and tempoName (both required) and only asks for tenant when the
user indicates or when querying a multi-tenant instance; replace the ambiguous
“instance and tenant” phrasing with clear instructions that callers must supply
tempoNamespace and tempoName and that tenant is conditional. Locate the prompt
generation in pkg/tools/prompt.go (the Tempo tools prompt text) and revise the
three lines beginning with “Do not query…” / “Ask the user…” to reflect these
exact identifier names and the conditional tenant requirement so tool handlers
receive valid inputs.
In `@pkg/toolset/tools/prometheus_client.go`:
- Around line 227-228: The http.Client created in prometheus_client.go (variable
httpClient) is missing a Timeout which can cause blocking; modify the
construction of httpClient to include a sensible Timeout (e.g., 15–30s or derive
from existing config) while keeping apiConfig.RoundTripper, then pass that
timed-out client into tempoclient.NewTempoLoader(url) so the Tempo loader cannot
hang indefinitely; update any related configuration or tests if they expect no
timeout.
In `@tests/e2e/e2e_test.go`:
- Around line 693-698: The test currently asserts slice order with require.Equal
on instances extracted from structuredContent, which can be flaky; change the
assertion to be order-insensitive by using require.ElementsMatch (or
assert.ElementsMatch) to compare the expected slice to instances (or
alternatively sort instances by "tempoName" before asserting). Update the
assertion that references structured, instances and the require.Equal call so
the test verifies membership regardless of element order.
---
Minor comments:
In @.gitignore:
- Around line 21-22: The .gitignore entry for ".claude" is ambiguous about
whether it is a file or directory; if ".claude" is a directory (common), update
the pattern to explicitly ignore the directory by changing the entry to use a
trailing slash (e.g., "/.claude/") so nested contents are ignored, otherwise
keep the existing "/.claude" if it is definitely a single file; locate the
".claude" line in the .gitignore and adjust the pattern accordingly.
In `@hack/tempo_multitenancy_openshift/README.md`:
- Around line 10-17: The fenced code block containing the shell commands
(kubectl create serviceaccount demo, TOKEN=$(kubectl create token demo), curl -G
-k ...) is missing a language identifier; update the opening fence to include
"bash" (i.e., change the triple backtick that begins the block to ```bash) so
markdownlint rule MD040 is satisfied and the block is correctly highlighted.
In `@TOOLS.md`:
- Around line 287-397: The generated Markdown tables have cells broken across
lines (e.g., parameter rows for end, start, tenant, and descriptions under
tempo_search_traces/tempo_search_tags/tempo_search_tag_values), which breaks
table structure; update the docs generator/template that emits these table rows
to collapse or replace internal newlines in cell content so each table cell is
emitted as a single-line cell (or explicitly use HTML <br> for intended line
breaks), e.g., sanitize the description strings for parameters like "end",
"start", "limit", "spss", "query" before rendering the table and ensure the
template joins multi-line descriptions into one logical cell to preserve valid
Markdown table rows.
---
Nitpick comments:
In `@hack/e2e/manifests/tracing/01_minio.yaml`:
- Line 43: The manifest uses an unpinned floating MinIO image string "image:
docker.io/minio/minio"; replace it with a pinned image tag or digest (for
example "image: docker.io/minio/minio:<specific-tag>" or a sha256 digest) to
prevent silent drift in e2e runs, updating the "image: docker.io/minio/minio"
entry in the 01_minio manifest to a concrete tag or digest and ensure the chosen
tag is recorded in CI/documentation.
In `@hack/tempo_multitenancy_openshift/00_operators.yaml`:
- Around line 20-24: Subscription entries currently use channel: stable and
installPlanApproval: Automatic which allows non-deterministic, unreviewed
operator upgrades; update both subscription blocks by replacing channel: stable
with a pinned/fixed channel name (not "stable") and set installPlanApproval:
Manual so upgrades require an install plan approval/review; apply this change to
the subscription for opentelemetry-product and the other subscription block
referenced in the diff.
In `@hack/tempo_multitenancy_openshift/02_minio.yaml`:
- Line 43: Replace the floating image reference "image: docker.io/minio/minio"
with a pinned tag or digest (e.g., docker.io/minio/minio:<tested-tag> or
docker.io/minio/minio@sha256:<digest>) in the container image field so the
manifest uses a reproducible, tested MinIO release; update the
Deploy/StatefulSet spec where "image" is set and document the chosen tag/digest
so future updates go through an explicit review process or automated dependency
updates.
In `@pkg/mcp/tools.go`:
- Around line 10-13: The comment for AllTools is outdated: update the function
doc above AllTools() to mention that tools are now merged from both
tools.AllTools() and tempo.AllTools(), and instruct contributors to add new tool
definitions in the correct registries (e.g., pkg/tools/definitions.go for core
tools and the tempo package's definitions file for tempo tools) so both MCP and
the Toolset remain in sync; reference the AllTools function and the calls
tools.AllTools() and tempo.AllTools() when making this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
andreasgerstmayr
force-pushed
the
tracing
branch
2 times, most recently
from
April 28, 2026 18:45
1cd9264 to
50bc7e2
Compare
Contributor
Author
Done. |
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (6)
hack/tempo_multitenancy_openshift/02_minio.yaml (1)
15-54: Optional: add readiness/liveness probes for faster/safer orchestration.For demo/e2e reliability, consider adding HTTP/TCP probes to ensure MinIO is actually ready before downstream components (OpenTelemetry Collector / Tempo) attempt to connect.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/tempo_multitenancy_openshift/02_minio.yaml` around lines 15 - 54, The Deployment "minio" currently lacks liveness/readiness probes; add a readinessProbe and livenessProbe to the container named "minio" so orchestration waits for MinIO to be healthy before routing traffic. Implement HTTP GET probes against the MinIO health endpoints (e.g. /minio/health/ready for readiness and /minio/health/live for liveness) on port 9000 with sensible timeouts and delays (initialDelaySeconds, periodSeconds, timeoutSeconds, failureThreshold) so the pod is marked ready only when MinIO is serving and restarted when unhealthy.pkg/traces/config.go (2)
50-56: Validate config during parsing for fail-fast behavior.Decoding succeeds even for invalid
auth_modevalues unless validation is guaranteed elsewhere. Callingcfg.Validate()here makes the parser self-contained and safer.Suggested change
func tempoToolsetParser(_ context.Context, primitive toml.Primitive, md toml.MetaData) (api.ExtendedConfig, error) { var cfg Config if err := md.PrimitiveDecode(primitive, &cfg); err != nil { return nil, err } + if err := cfg.Validate(); err != nil { + return nil, err + } return &cfg, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/traces/config.go` around lines 50 - 56, tempoToolsetParser currently decodes TOML into a Config but doesn't validate it, allowing invalid values like wrong auth_mode to pass; after md.PrimitiveDecode and before returning, call cfg.Validate() (or the appropriate method on Config) and return any validation error so the parser fails fast; update tempoToolsetParser to return &cfg only if cfg.Validate() returns nil.
58-67: Return a copy of defaults instead of the shared default pointer.Returning
DefaultConfigdirectly can couple callers through shared mutable state if any code mutates the returned config.Suggested change
func GetConfig(params api.ToolHandlerParams) *Config { if cfg, ok := params.GetToolsetConfig(ToolsetName); ok { if tempoCfg, ok := cfg.(*Config); ok { return tempoCfg } } // Return default config if not found - return DefaultConfig + cfg := *DefaultConfig + return &cfg }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/traces/config.go` around lines 58 - 67, GetConfig currently returns the shared DefaultConfig pointer which risks shared mutable state; change the fallback to return a fresh copy instead. In the GetConfig function, when no toolset config is found, create a new Config by copying the value pointed to by DefaultConfig (e.g., cfg := *DefaultConfig; return &cfg) so callers get an independent instance; reference GetConfig and DefaultConfig to locate where to apply the change.hack/e2e/setup-cluster.sh (1)
70-76: Make retry exhaustion fail explicitly with a clear error.If all apply attempts fail, the script currently proceeds and fails later on waits with less direct context. Add an explicit post-loop failure check.
Suggested refactor
echo "==> Setting up MinIO, OTEL collector, Tempo and example traces" # simple retry loop to catch issues where the operator pod is deployed, but the webhook is not ready yet +applied=false for i in $(seq 1 3); do - kubectl apply -f "${SCRIPT_DIR}/manifests/tracing" && break + if kubectl apply -f "${SCRIPT_DIR}/manifests/tracing"; then + applied=true + break + fi echo " Retrying in 10s... (attempt $i/3)" sleep 10 done +if [ "$applied" != "true" ]; then + echo "ERROR: failed to apply tracing manifests after 3 attempts" + exit 1 +fi kubectl -n obs-mcp-tracing wait --for=condition=Ready tempostack/tempo1 --timeout=5m🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e/setup-cluster.sh` around lines 70 - 76, The retry loop that runs kubectl apply -f "${SCRIPT_DIR}/manifests/tracing" needs to explicitly fail if all attempts exhaust; add a success flag (e.g., success=0 before the for-loop and set success=1 inside the loop right after a successful apply/before break) and after the loop check the flag (if success is still 0) then echo a clear error like "Failed to apply tracing manifests after 3 attempts" and exit 1; update the loop around kubectl apply in hack/e2e/setup-cluster.sh so the script stops with a descriptive error rather than proceeding when all retries fail.pkg/traces/list_instances_test.go (1)
24-33: Make instance assertions order-independent to prevent flaky tests.Using positional checks (
output.Instances[0],output.Instances[1]) can intermittently fail when list order changes. Prefer matching by stable keys (namespace/name) and then asserting fields.Suggested refactor
- inst := output.Instances[0] - require.Equal(t, "ns1", inst.Namespace) - require.Equal(t, "stack1", inst.Name) - require.Equal(t, []string{"tenant-a", "tenant-b"}, inst.Tenants) - require.Equal(t, "Ready", inst.Status) - - inst2 := output.Instances[1] - require.Equal(t, "ns2", inst2.Namespace) - require.Equal(t, "stack2", inst2.Name) + var foundStack1, foundStack2 bool + for _, inst := range output.Instances { + switch inst.Namespace + "/" + inst.Name { + case "ns1/stack1": + foundStack1 = true + require.Equal(t, []string{"tenant-a", "tenant-b"}, inst.Tenants) + require.Equal(t, "Ready", inst.Status) + case "ns2/stack2": + foundStack2 = true + } + } + require.True(t, foundStack1, "expected instance ns1/stack1") + require.True(t, foundStack2, "expected instance ns2/stack2")As per coding guidelines,
**: "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/traces/list_instances_test.go` around lines 24 - 33, The test uses positional assertions on output.Instances (accessing output.Instances[0] and [1]) which is flaky; instead locate instances by a stable key (e.g., fmt.Sprintf("%s/%s", inst.Namespace, inst.Name) or "namespace/name") or build a map from output.Instances keyed by that string, then assert fields (Namespace, Name, Tenants, Status) against the expected entry for "ns1/stack1" and "ns2/stack2"; update the assertions that reference inst and inst2 to look up the instance in the map (or search by key) before checking Tenants and Status so order changes won't break the test.pkg/traces/get_trace_by_id.go (1)
64-77: Validatestart <= endbefore calling Tempo.Fail fast on inverted ranges to avoid avoidable downstream errors and clearer user feedback.
As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."Proposed patch
start, err := parseDate(tools.GetString(args, "start", "")) if err != nil { return GetTraceByIDOutput{}, fmt.Errorf("invalid start time: %v", err) } end, err := parseDate(tools.GetString(args, "end", "")) if err != nil { return GetTraceByIDOutput{}, fmt.Errorf("invalid end time: %v", err) } + if start != 0 && end != 0 && start > end { + return GetTraceByIDOutput{}, fmt.Errorf("start time must be before end time") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/traces/get_trace_by_id.go` around lines 64 - 77, After parsing start and end with parseDate (used above to build tempoclient.QueryV2Options), add a validation that start is not after end and return a GetTraceByIDOutput{} with a clear error if the range is inverted (e.g., "start time must be <= end time"). Place this check after parsing both dates and before constructing opts so the function fails fast on invalid ranges and avoids calling Tempo with an inverted window.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@evals/lightspeed/evals.yaml`:
- Around line 255-295: The eval expects tempo_search_traces calls without a
tenant which breaks with tenant-scoped Tempo instances; update the two
occurrences of expected_tool_calls that list tempo_search_traces (the one under
the first conversation and the one under conversation_group_id
"tempo-latency-investigation" turn_id "turn-1") to include the tenant argument
consistent with runtime multitenant rules (add a tenant: <tenant-name or
placeholder> field alongside tempoNamespace, tempoName, query, start/end or
server_label) so the eval is unambiguous in tenant-scoped environments.
In `@hack/e2e/manifests/tracing/01_minio.yaml`:
- Around line 38-43: The manifest currently hardcodes MINIO_ACCESS_KEY and
MINIO_SECRET_KEY in the Deployment env (env names MINIO_ACCESS_KEY,
MINIO_SECRET_KEY) which duplicates Secret data; remove the literal value fields
and change the container env entries to use secretKeyRef to pull the values from
the existing Secret (store the keys in the Secret data under matching keys),
e.g. replace env value: "tempo"/"supersecret" with env: secretKeyRef: name:
<your-secret-name> key: MINIO_ACCESS_KEY (and similarly for MINIO_SECRET_KEY);
ensure you update both occurrences (lines around the two env blocks) so the
Secret is the single source of truth.
- Around line 30-53: The MinIO container "minio" currently lacks pod/container
securityContext settings; update the Pod spec in 01_minio.yaml to enforce
non-root and minimal privileges by adding a podSecurityContext (e.g.,
runAsNonRoot: true, runAsUser: 1000, fsGroup) and a container securityContext
for the "minio" container that sets allowPrivilegeEscalation: false,
readOnlyRootFilesystem: true (if compatible), capabilities.drop: ["ALL"], and
seccompProfile: { type: "RuntimeDefault" }; ensure these keys are added under
the existing spec -> containers -> name: minio and at the top-level spec for the
pod so the container runs as a non-root, non-privileged process.
In `@hack/e2e/manifests/tracing/04_testdata_k6.yaml`:
- Around line 17-23: The pod's container "k6-tracing" is running with default
privileges; update the manifest to harden the container securityContext by
setting runAsNonRoot: true (and optionally runAsUser: 1000), set
allowPrivilegeEscalation: false, drop all capabilities (capabilities.drop:
["ALL"]) and add a seccompProfile with type: RuntimeDefault (either under the
pod.spec.securityContext or under the container.securityContext for the
"k6-tracing" container); ensure these keys are added alongside the existing
container definition so the k6-tracing container runs unprivileged with no
privilege escalation and default seccomp enforcement.
In `@hack/tempo_multitenancy_openshift/02_minio.yaml`:
- Around line 69-79: Secret/minio currently contains hardcoded sensitive
credentials in stringData (access_key_secret: supersecret and access_key_id:
tempo) which should not be committed; replace these values with non-sensitive
placeholders (e.g., CHANGE_ME) in the manifest and update any duplicated usages
in the Deployment env to reference the same placeholder or a SecretKeyRef, then
implement secret provisioning at deploy time (via kustomize
secretGenerator/vars, Helm values, or an external Secret created out-of-band) so
the real access_key_id/ access_key_secret are injected during deployment rather
than stored in the repo.
- Around line 32-42: Replace the hard-coded MINIO_ACCESS_KEY and
MINIO_SECRET_KEY env.value entries in the Deployment container with secret
references: change the env for MINIO_ACCESS_KEY and MINIO_SECRET_KEY to use
valueFrom.secretKeyRef pointing to the existing Secret named "minio" and the
corresponding secret keys (e.g., MINIO_ACCESS_KEY and MINIO_SECRET_KEY); update
the Deployment's container env entries (the MINIO_ACCESS_KEY and
MINIO_SECRET_KEY env vars) to use secretKeyRef so the pod reads credentials from
Secret/minio instead of env.value.
- Around line 15-54: Add a hardened container securityContext to the minio
Deployment's container named "minio": set
securityContext.readOnlyRootFilesystem: true,
securityContext.allowPrivilegeEscalation: false, securityContext.runAsNonRoot:
true and securityContext.runAsUser: 1000, and drop all capabilities via
securityContext.capabilities.drop: ["ALL"]; also mount a writable tmp dir (add a
volumeMount for /tmp and a corresponding emptyDir volume named "tmp") so the
image can write to /tmp while keeping the root filesystem read-only, leaving the
existing /storage PVC mount intact.
- Line 43: Update the MinIO Deployment to pin the container image to a specific
release tag instead of the untagged image "docker.io/minio/minio" (e.g.,
"docker.io/minio/minio:RELEASE.2025-01-01T00-00-00Z") and rename the deprecated
environment vars: replace MINIO_ACCESS_KEY with MINIO_ROOT_USER and
MINIO_SECRET_KEY with MINIO_ROOT_PASSWORD in the container spec's env list so
the deployment uses the current MinIO root credentials naming.
In `@hack/tempo_multitenancy_openshift/05_testdata_k6.yaml`:
- Around line 17-23: The deployment's container "k6-tracing" lacks
securityContext hardening; add a podSecurityContext and a container-level
securityContext to enforce non-root and restrict privileges: set
podSecurityContext.runAsNonRoot: true and runAsUser to a non-root UID (e.g.,
1000), and in the container "k6-tracing" set runAsNonRoot: true, runAsUser: same
UID, allowPrivilegeEscalation: false, capabilities.drop: ["ALL"], privileged:
false, readOnlyRootFilesystem: true and set seccompProfile.type: RuntimeDefault
(or Localhost if your cluster requires). These changes ensure the container
cannot run as root and drops unnecessary capabilities and privilege escalation.
In `@hack/tempo_multitenancy_openshift/06_testdata_hotrod.yaml`:
- Around line 16-39: Add explicit pod and container security contexts for the
hotrod container to prevent root-like execution: under spec add pod-level
securityContext (runAsNonRoot: true, runAsUser: 1000, runAsGroup: 1000) and
under the hotrod container (spec.containers[] name: hotrod) add
container.securityContext with runAsNonRoot: true, runAsUser: 1000,
allowPrivilegeEscalation: false, readOnlyRootFilesystem: true, and
securityContext.capabilities.drop: ["ALL"]; if the app requires temporary write
access, do not make the root FS writable—add a volumes entry with an emptyDir
and a matching volumeMount in the hotrod container (e.g., mountPath: /tmp) and
keep readOnlyRootFilesystem true while giving writable access only to that
emptyDir.
In `@pkg/mcp/server.go`:
- Around line 67-86: The middleware in mcpServer.AddReceivingMiddleware
currently logs raw tool arguments in the case handling *mcp.CallToolParamsRaw
(p.Arguments), which may expose sensitive data; change the logic in that handler
to unmarshal p.Arguments into a map as now but then build slog.Attr entries only
for a small whitelist of safe keys (e.g., "trace_id", "tenant" should be
redacted or omitted, and free-form query fields like "query"/"traceql" should be
masked or replaced with a length/placeholder), otherwise log a redacted summary
such as the argument count or masked values; keep logging p.Name and method, and
use slog.Any/slog.String as before but only for the allowed keys or masked
placeholders so full raw arguments are never emitted.
- Around line 109-126: When enabling the traces toolset, add a nil-check for
opts.Tempo and return a clear setup error before registering handlers so we fail
fast instead of panicking inside traces/common.go; specifically in the block
that checks slices.Contains(opts.Toolsets, ToolsetTraces) validate opts.Tempo !=
nil (or that required fields like UseRoute are present) and return an error
(e.g. "tempo config required for traces toolset") if missing, before calling
getTempoHTTPClient, creating dynamicClient, or calling mcp.AddTool with
traces.ToMCPHandler / tempoToolset.*Handler.
In `@pkg/traces/common.go`:
- Around line 55-63: getTempoClient currently calls discovery.ListInstances
(which does a cluster-wide list) then filters with findInstanceByName; change
this to resolve the Tempo instance directly by namespace and name to avoid
cluster-wide list/rbac and extra latency. Update the call in getTempoClient to
use a new or existing namespace-scoped discovery function (e.g.,
discovery.GetInstance(namespace, name) or
discovery.ListInstancesInNamespace(namespace,...)) instead of
discovery.ListInstances, or add caching in the discovery package to reuse
results; adjust error handling around findInstanceByName/get method accordingly
and update pkg/traces/discovery/discovery.go to provide the namespace-scoped
lookup used by getTempoClient.
In `@pkg/traces/search_tag_values.go`:
- Around line 79-99: Validate the parsed dates and numeric inputs before
building tempoclient.SearchTagValuesV2Options: after calling parseDate for start
and end (and after obtaining limit and maxStaleValues via tools.GetInt), return
an error if start is after end (start > end) and return errors if limit or
maxStaleValues are negative; update the function that constructs the
SearchTagValuesV2Options (use the existing start, end, limit, maxStaleValues
variables) to only proceed when these checks pass and include clear error
messages like "invalid time range: start is after end" and "invalid limit: must
be non-negative" to prevent sending invalid queries to Tempo.
In `@pkg/traces/search_tags.go`:
- Around line 77-99: The handler currently accepts start/end and numeric args
without validating ordering or non-negativity; add checks after parsing (using
parseDate and the retrieved start/end) to return an error if start is after end,
and validate limit and maxStaleValues (from tools.GetInt) to ensure they are
non-negative, returning a descriptive error before constructing
tempoclient.SearchTagsV2Options; update the function that builds opts
(SearchTagsV2Options) to only proceed when these validations pass.
In `@pkg/traces/search_traces.go`:
- Around line 102-121: After parsing start and end with parseDate and reading
limit and spss via tools.GetInt, validate the inputs before building
tempoclient.SearchOptions: return a descriptive error (SearchTracesOutput{},
fmt.Errorf(...)) if start is after end, if limit is negative, or if spss is
negative so the call to Tempo is not made with invalid ranges; place these
checks immediately after obtaining start, end, limit and spss and before
constructing tempoclient.SearchOptions.
In `@pkg/traces/tempo/client.go`:
- Around line 39-52: The current read uses io.LimitReader which silently
truncates large responses; change the read to attempt reading up to
maxResponseSize+1 (e.g., via io.LimitReader(resp.Body, maxResponseSize+1)) and
if the resulting bodyBytes length is greater than maxResponseSize return a clear
error instead of treating the truncated payload as success; keep checks using
resp.StatusCode, resp.Header.Get("Content-Type") and the variables bodyBytes,
resp and maxResponseSize unchanged but add the explicit truncation-detection and
error path.
In `@TOOLS.md`:
- Around line 287-290: The generated TOOLS.md table rows are broken by multiline
cell content; update the docs generator to replace embedded newlines in table
cells with HTML <br> before emitting rows so pipes and Markdown table structure
stay intact. Locate the table emission code (e.g., functions named
generateMarkdownTable, renderTableRow, renderCell or writeRow) and sanitize each
cell via a helper (e.g., sanitizeCellContent) that converts "\n" to "<br>" (and
collapses trailing/leading whitespace) prior to joining cells with "|" and
writing the row; ensure this change is applied wherever tables are emitted so
rows like the Tempo parameter rows (and the other ranges mentioned) render as
single-line cells.
- Around line 295-297: The markdown table shows empty type cells for `trace` and
`tagValues`; update the schema generator so that when a field's type would
render as empty it maps to a concrete type (e.g., "object" or "any") instead of
blank; locate the code that builds the output schema for these fields (search
for the logic that renders field rows or the mapping for `trace`/`tagValues` in
the docs generator) and default empty/undefined types to "object" (or "any") so
the TOOLS.md table rows for `trace` and `tagValues` display a non-empty type.
---
Nitpick comments:
In `@hack/e2e/setup-cluster.sh`:
- Around line 70-76: The retry loop that runs kubectl apply -f
"${SCRIPT_DIR}/manifests/tracing" needs to explicitly fail if all attempts
exhaust; add a success flag (e.g., success=0 before the for-loop and set
success=1 inside the loop right after a successful apply/before break) and after
the loop check the flag (if success is still 0) then echo a clear error like
"Failed to apply tracing manifests after 3 attempts" and exit 1; update the loop
around kubectl apply in hack/e2e/setup-cluster.sh so the script stops with a
descriptive error rather than proceeding when all retries fail.
In `@hack/tempo_multitenancy_openshift/02_minio.yaml`:
- Around line 15-54: The Deployment "minio" currently lacks liveness/readiness
probes; add a readinessProbe and livenessProbe to the container named "minio" so
orchestration waits for MinIO to be healthy before routing traffic. Implement
HTTP GET probes against the MinIO health endpoints (e.g. /minio/health/ready for
readiness and /minio/health/live for liveness) on port 9000 with sensible
timeouts and delays (initialDelaySeconds, periodSeconds, timeoutSeconds,
failureThreshold) so the pod is marked ready only when MinIO is serving and
restarted when unhealthy.
In `@pkg/traces/config.go`:
- Around line 50-56: tempoToolsetParser currently decodes TOML into a Config but
doesn't validate it, allowing invalid values like wrong auth_mode to pass; after
md.PrimitiveDecode and before returning, call cfg.Validate() (or the appropriate
method on Config) and return any validation error so the parser fails fast;
update tempoToolsetParser to return &cfg only if cfg.Validate() returns nil.
- Around line 58-67: GetConfig currently returns the shared DefaultConfig
pointer which risks shared mutable state; change the fallback to return a fresh
copy instead. In the GetConfig function, when no toolset config is found, create
a new Config by copying the value pointed to by DefaultConfig (e.g., cfg :=
*DefaultConfig; return &cfg) so callers get an independent instance; reference
GetConfig and DefaultConfig to locate where to apply the change.
In `@pkg/traces/get_trace_by_id.go`:
- Around line 64-77: After parsing start and end with parseDate (used above to
build tempoclient.QueryV2Options), add a validation that start is not after end
and return a GetTraceByIDOutput{} with a clear error if the range is inverted
(e.g., "start time must be <= end time"). Place this check after parsing both
dates and before constructing opts so the function fails fast on invalid ranges
and avoids calling Tempo with an inverted window.
In `@pkg/traces/list_instances_test.go`:
- Around line 24-33: The test uses positional assertions on output.Instances
(accessing output.Instances[0] and [1]) which is flaky; instead locate instances
by a stable key (e.g., fmt.Sprintf("%s/%s", inst.Namespace, inst.Name) or
"namespace/name") or build a map from output.Instances keyed by that string,
then assert fields (Namespace, Name, Tenants, Status) against the expected entry
for "ns1/stack1" and "ns2/stack2"; update the assertions that reference inst and
inst2 to look up the instance in the map (or search by key) before checking
Tenants and Status so order changes won't break the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ce537af5-daca-4afe-954c-ef51b0df2e60
📒 Files selected for processing (51)
.gitignoreTOOLS.mdcmd/obs-mcp/main.goevals/lightspeed/evals.yamlevals/lightspeed/system.yamlevals/mcpchecker/eval.yamlevals/mcpchecker/tasks/traces/latency-investigation.yamlevals/mcpchecker/tasks/traces/search-error-traces.yamlgo.modhack/e2e/manifests/tracing/00_namespace.yamlhack/e2e/manifests/tracing/01_minio.yamlhack/e2e/manifests/tracing/02_otel.yamlhack/e2e/manifests/tracing/03_tempo.yamlhack/e2e/manifests/tracing/04_testdata_k6.yamlhack/e2e/setup-cluster.shhack/tempo_multitenancy_openshift/00_operators.yamlhack/tempo_multitenancy_openshift/01_project.yamlhack/tempo_multitenancy_openshift/02_minio.yamlhack/tempo_multitenancy_openshift/03_otel.yamlhack/tempo_multitenancy_openshift/04_tempo.yamlhack/tempo_multitenancy_openshift/05_testdata_k6.yamlhack/tempo_multitenancy_openshift/06_testdata_hotrod.yamlhack/tempo_multitenancy_openshift/README.mdmanifests/kubernetes/01_service_account.yamlmanifests/kubernetes/03_deployment.yamlpkg/mcp/auth.gopkg/mcp/server.gopkg/mcp/tools.gopkg/tools/handlers.gopkg/tools/prompt.gopkg/tools/tooldef.gopkg/toolset/tools/prometheus_client.gopkg/toolset/toolset.gopkg/traces/common.gopkg/traces/common_test.gopkg/traces/config.gopkg/traces/discovery/discovery.gopkg/traces/discovery/sanitize.gopkg/traces/discovery/types.gopkg/traces/get_trace_by_id.gopkg/traces/list_instances.gopkg/traces/list_instances_test.gopkg/traces/search_tag_values.gopkg/traces/search_tags.gopkg/traces/search_traces.gopkg/traces/tempo/client.gopkg/traces/tempo/client_test.gopkg/traces/tempo/loader.gopkg/traces/tempo/loader_test.gopkg/traces/toolset.gotests/e2e/e2e_test.go
✅ Files skipped from review due to trivial changes (7)
- .gitignore
- hack/e2e/manifests/tracing/00_namespace.yaml
- hack/tempo_multitenancy_openshift/01_project.yaml
- pkg/mcp/auth.go
- evals/mcpchecker/eval.yaml
- hack/tempo_multitenancy_openshift/03_otel.yaml
- hack/e2e/manifests/tracing/02_otel.yaml
🚧 Files skipped from review as they are similar to previous changes (12)
- manifests/kubernetes/03_deployment.yaml
- pkg/tools/prompt.go
- evals/mcpchecker/tasks/traces/search-error-traces.yaml
- manifests/kubernetes/01_service_account.yaml
- go.mod
- pkg/toolset/tools/prometheus_client.go
- hack/e2e/manifests/tracing/03_tempo.yaml
- hack/tempo_multitenancy_openshift/00_operators.yaml
- evals/mcpchecker/tasks/traces/latency-investigation.yaml
- cmd/obs-mcp/main.go
- hack/tempo_multitenancy_openshift/04_tempo.yaml
- tests/e2e/e2e_test.go
andreasgerstmayr
force-pushed
the
tracing
branch
from
April 30, 2026 10:46
50bc7e2 to
5063795
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
pkg/traces/tempo/client.go (1)
22-35: Consider enforcing timeout at the client level for defense-in-depth.This client performs external Tempo calls but relies on callers to set a timeout on the
http.Client. While current production callers (pkg/mcp/auth.goandpkg/toolset/tools/prometheus_client.go) all explicitly setTimeout: tempoclient.RequestTimeoutbefore construction, enforcing it here would prevent accidental misuse if the code is reused or refactored. The suggested approach (validating and cloning if timeout is unset) is reasonable if you want stricter guarantees, but is not required given the current call paths are compliant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/traces/tempo/client.go` around lines 22 - 35, The NewTempoClient constructor should enforce a default timeout for defense-in-depth: inside NewTempoClient check the provided *http.Client's Timeout and if it is zero, clone the client (or create a shallow copy) and set its Timeout to RequestTimeout before assigning it to TempoClient.httpClient; update NewTempoClient and document that TempoClient always uses a non-zero timeout even if callers omit it (referencing NewTempoClient, TempoClient.httpClient, and RequestTimeout).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/obs-mcp/main.go`:
- Around line 145-151: The parseToolsets function currently fails to accept
values with surrounding whitespace; fix it by trimming each token before
validation and assignment: in parseToolsets, call strings.TrimSpace on the loop
variable p (use the trimmed value for slices.Contains against
mcpserver.AllToolsets and for populating the result slice) so inputs like
"metrics, traces" are accepted; keep the same error path using the trimmed token
in the log.Fatalf message.
In `@pkg/mcp/server.go`:
- Line 62: The current construction always injects traces.ServerPrompt into the
Instructions string; change it to conditionally include traces.ServerPrompt only
when ToolsetTraces is enabled by building the instructions piecewise (e.g.,
start with tools.ServerPrompt and append traces.ServerPrompt only if the toolset
flag ToolsetTraces is set), replacing the fmt.Sprintf("%s\n%s\n",
tools.ServerPrompt, traces.ServerPrompt) usage so the model only receives
Tempo/traces instructions when traces tooling is enabled.
In `@pkg/toolset/tools/prometheus_client.go`:
- Around line 223-236: The shared promapi.DefaultRoundTripper is being mutated
(in createAPIConfigWithToken) which leaks TLSClientConfig across callers; change
createAPIConfigWithToken (and any code that sets apiConfig.RoundTripper) to
first clone the default transport (e.g. t := promapi.DefaultRoundTripper.Clone()
or create a shallow copy of the http.Transport) and then modify
t.TLSClientConfig, and assign apiConfig.RoundTripper = t so
NewTempoClient/buildAPIConfig uses a per-request transport rather than mutating
the global promapi.DefaultRoundTripper.
In `@pkg/traces/discovery/discovery.go`:
- Around line 55-78: The current loop in discovery (where
runtime.DefaultUnstructuredConverter.FromUnstructured converts each item into
TempoStack and resolveBaseURL is called) aborts the entire tempo_list_instances
operation on the first parse or resolve error; change this to skip the bad Tempo
resource instead: in the loop around list.Items (TempoStack conversion and
resolveBaseURL calls) catch errors, append a descriptive error to a local slice
(e.g., errs) and continue to the next item rather than returning immediately,
collect successful instance entries into the results slice, and after the loop
return the results plus an aggregated error (e.g., join errs into one fmt.Errorf
or wrap with multierror) so callers get partial results and visibility into
failures.
In `@tests/e2e/e2e_test.go`:
- Line 684: The MCP request ID is hardcoded as 22 in the CallTool calls (e.g.,
resp, err := mcpClient.CallTool(t, 22, "tempo_list_instances", ...)); change
these to unique IDs per test invocation to avoid collisions—either generate IDs
from the test name (e.g., hash or parse t.Name()), use a per-test counter, or
call a helper like nextMCPRequestID() and replace the literal 22 in both
locations (and any other CallTool usages) so each test uses a distinct integer
request id.
- Around line 695-698: The test currently uses an order-dependent require.Equal
on the discovered "instances" slice; replace it with an order-insensitive
comparison (e.g. require.ElementsMatch) so the test doesn't flake if Tempo
ordering changes. Update the assertion that references require.Equal(t,
[]any{...}, instances) to require.ElementsMatch(t,
[]any{map[string]any{"kind":"TempoStack", "tempoNamespace":"obs-mcp-tracing",
"tempoName":"tempo1", ...}, map[string]any{"kind":"TempoStack",
"tempoNamespace":"obs-mcp-tracing", "tempoName":"tempo2", ...}}, instances) (or
alternatively sort instances by "tempoName" first) so the comparison is
unordered; target the assertion in the test that constructs the expected []any
and the variable instances.
---
Nitpick comments:
In `@pkg/traces/tempo/client.go`:
- Around line 22-35: The NewTempoClient constructor should enforce a default
timeout for defense-in-depth: inside NewTempoClient check the provided
*http.Client's Timeout and if it is zero, clone the client (or create a shallow
copy) and set its Timeout to RequestTimeout before assigning it to
TempoClient.httpClient; update NewTempoClient and document that TempoClient
always uses a non-zero timeout even if callers omit it (referencing
NewTempoClient, TempoClient.httpClient, and RequestTimeout).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7820c384-3ebe-4594-a0d6-95ddc8f379bc
📒 Files selected for processing (51)
.gitignoreTOOLS.mdcmd/obs-mcp/main.goevals/lightspeed/evals.yamlevals/lightspeed/system.yamlevals/mcpchecker/eval.yamlevals/mcpchecker/tasks/traces/latency-investigation.yamlevals/mcpchecker/tasks/traces/search-error-traces.yamlgo.modhack/e2e/manifests/tracing/00_namespace.yamlhack/e2e/manifests/tracing/01_minio.yamlhack/e2e/manifests/tracing/02_otel.yamlhack/e2e/manifests/tracing/03_tempo.yamlhack/e2e/manifests/tracing/04_testdata_k6.yamlhack/e2e/setup-cluster.shhack/tempo_multitenancy_openshift/00_operators.yamlhack/tempo_multitenancy_openshift/01_project.yamlhack/tempo_multitenancy_openshift/02_minio.yamlhack/tempo_multitenancy_openshift/03_otel.yamlhack/tempo_multitenancy_openshift/04_tempo.yamlhack/tempo_multitenancy_openshift/05_testdata_k6.yamlhack/tempo_multitenancy_openshift/06_testdata_hotrod.yamlhack/tempo_multitenancy_openshift/README.mdmanifests/kubernetes/01_service_account.yamlmanifests/kubernetes/03_deployment.yamlpkg/mcp/auth.gopkg/mcp/server.gopkg/mcp/tools.gopkg/tools/handlers.gopkg/tools/tooldef.gopkg/toolset/tools/prometheus_client.gopkg/toolset/toolset.gopkg/traces/common.gopkg/traces/common_test.gopkg/traces/config.gopkg/traces/discovery/discovery.gopkg/traces/discovery/sanitize.gopkg/traces/discovery/types.gopkg/traces/get_trace_by_id.gopkg/traces/list_instances.gopkg/traces/list_instances_test.gopkg/traces/prompt.gopkg/traces/search_tag_values.gopkg/traces/search_tags.gopkg/traces/search_traces.gopkg/traces/tempo/client.gopkg/traces/tempo/client_test.gopkg/traces/tempo/loader.gopkg/traces/tempo/loader_test.gopkg/traces/toolset.gotests/e2e/e2e_test.go
✅ Files skipped from review due to trivial changes (12)
- hack/e2e/manifests/tracing/00_namespace.yaml
- .gitignore
- manifests/kubernetes/01_service_account.yaml
- pkg/traces/prompt.go
- pkg/tools/handlers.go
- hack/tempo_multitenancy_openshift/01_project.yaml
- pkg/tools/tooldef.go
- pkg/mcp/auth.go
- pkg/traces/common_test.go
- evals/mcpchecker/tasks/traces/latency-investigation.yaml
- pkg/traces/discovery/sanitize.go
- evals/mcpchecker/eval.yaml
🚧 Files skipped from review as they are similar to previous changes (19)
- evals/lightspeed/system.yaml
- pkg/toolset/toolset.go
- pkg/traces/list_instances_test.go
- evals/mcpchecker/tasks/traces/search-error-traces.yaml
- pkg/mcp/tools.go
- go.mod
- pkg/traces/tempo/client_test.go
- pkg/traces/tempo/loader_test.go
- pkg/traces/list_instances.go
- hack/tempo_multitenancy_openshift/00_operators.yaml
- pkg/traces/search_tag_values.go
- pkg/traces/get_trace_by_id.go
- pkg/traces/search_tags.go
- hack/e2e/manifests/tracing/03_tempo.yaml
- manifests/kubernetes/03_deployment.yaml
- hack/e2e/setup-cluster.sh
- pkg/traces/discovery/types.go
- hack/tempo_multitenancy_openshift/04_tempo.yaml
- hack/tempo_multitenancy_openshift/03_otel.yaml
This was referenced Apr 30, 2026
andreasgerstmayr
force-pushed
the
tracing
branch
2 times, most recently
from
April 30, 2026 11:17
b1ad0ba to
540a920
Compare
iNecas
reviewed
May 4, 2026
Contributor
Author
It was a timing issue (worked fine locally), I've fixed it now.
Yes (tested with |
Contributor
|
Looks good to me, but letting @slashpai to do the final approve |
iNecas
reviewed
May 11, 2026
| SLEEP=10 | ||
|
|
||
| echo "==> Waiting for traces to appear at ${URL}..." | ||
| for i in $(seq 1 "${MAX_ATTEMPTS}"); do |
Contributor
There was a problem hiding this comment.
one small comment, creating a pod for a single curl iteration seems a bit wasteful? Could the loop be moved into the pod execution?
Member
There was a problem hiding this comment.
@andreasgerstmayr I think this comment needs to be addressed but can be a follow-up also
Member
|
/test e2e-obs-mcp-gcp |
Member
|
Contributor
Author
|
/test e2e-obs-mcp-gcp |
|
@andreasgerstmayr: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Member
|
/test e2e-obs-mcp-gcp |
Member
|
I rebased the branch since this branch was behind main very much and attempting to run with fix |
Member
|
/test e2e-obs-mcp-gcp |
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
Member
|
with debug logs we could get the culprit, thanks @andreasgerstmayr :) we can use |
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
Contributor
Author
|
/test e2e-obs-mcp-gcp |
1 similar comment
Member
|
/test e2e-obs-mcp-gcp |
iNecas
approved these changes
May 13, 2026
Member
|
Thank you @andreasgerstmayr for the patience on this :) /lgtm |
Ari4ka
approved these changes
May 13, 2026
|
@Ari4ka: changing LGTM is restricted to collaborators DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andreasgerstmayr, Ari4ka, iNecas The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add Tempo tools:
tempo_list_instancestempo_search_tracestempo_get_trace_by_idtempo_search_tagstempo_search_tag_values