Skip to content

chore: correct vm ui link and include in json benchmark result#3234

Merged
chatton merged 1 commit intomainfrom
cian/fix-vm-ui-url
Apr 8, 2026
Merged

chore: correct vm ui link and include in json benchmark result#3234
chatton merged 1 commit intomainfrom
cian/fix-vm-ui-url

Conversation

@chatton
Copy link
Copy Markdown
Contributor

@chatton chatton commented Apr 8, 2026

Overview

This PR includes the correct link for vm traces. When a benchmark runs, it will include the timespan window to look for traces.

We will then be able to query this via a frontend or manually.

Summary by CodeRabbit

  • New Features
    • Benchmark results now include direct links to trace UI data, allowing users to inspect detailed trace information for each benchmark execution.
    • Enhanced trace link generation with improved timestamp precision and time range parameters for more accurate trace data correlation.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 8, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e7abc15a-96b4-47b1-bdb6-0b018575a75f

📥 Commits

Reviewing files that changed from the base of the PR and between 536f57e and 3a15775.

📒 Files selected for processing (4)
  • test/e2e/benchmark/result.go
  • test/e2e/benchmark/run_result.go
  • test/e2e/benchmark/suite_test.go
  • test/e2e/benchmark/traces.go

📝 Walkthrough

Walkthrough

The changes add traces URL tracking to benchmark result structures by updating the trace provider interface to accept an end timestamp parameter, modifying URL generation logic in the Victoria Metrics implementation, and propagating the computed traces URL through result collection and storage.

Changes

Cohort / File(s) Summary
Traces Interface & Implementation
test/e2e/benchmark/traces.go
Updated traceProvider interface method signature to accept end time.Time parameter. Modified victoriaTraceProvider.uiURL to build traces UI links using new query format with computed range_input and UTC-formatted end_input based on the provided timestamp.
Result Data Structures
test/e2e/benchmark/result.go, test/e2e/benchmark/run_result.go
Added tracesURL field to traceResult struct and TracesURL JSON-tagged field to runResult struct. Extended fieldDescriptions() map with human-readable description for the new traces_url field.
Result Collection
test/e2e/benchmark/suite_test.go
Updated collectTraces to call e.traces.uiURL with time.Now() timestamp and assign returned traces URL to tr.tracesURL in addition to logging.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • alpe

Poem

🐰 A trace of time, now captured true,
Timestamps guide the rabbit's view,
Through Victoria's queries, URLs bloom,
Benchmark results light every room! 🌿✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: correcting the VM UI link and including it in the JSON benchmark result output.
Description check ✅ Passed The description adequately explains the change (correct VM traces link with timespan) and the intent (enabling manual or frontend trace queries), covering the essential information.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch cian/fix-vm-ui-url

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 8, 2026

Claude finished @chatton's task in 1m 28s —— View job


Code Review

  • Gather context and read changed files
  • Review code quality & architecture
  • Review error handling & resilience
  • Review security
  • Review performance & resource efficiency
  • Review testing & reliability
  • Post final review

Summary

This is a small, focused change that fixes the VMUI URL format for VictoriaTraces and propagates the link into the JSON benchmark result. The logic is straightforward and clean.


Observations

traces.gouiURL signature change

The old URL format used the /#/query route; the new format uses the /?#/?g0.expr= Explore-style route which is the correct VictoriaMetrics VMUI format for graph panel exploration. The end time.Time parameter is a good design choice — it makes the URL deterministic relative to when trace collection ended, avoiding a silent dependency on time.Now() inside the method.

Minor: rangeInput is not URL-encoded

// traces.go:71-75
rangeInput := end.Sub(v.startTime).Round(time.Second).String()
endInput := end.UTC().Format("2006-01-02T15:04:05")
return fmt.Sprintf("%s/select/vmui/?#/?g0.expr=%s&g0.range_input=%s&g0.end_input=%s",
    strings.TrimRight(v.queryURL, "/"),
    neturl.QueryEscape(query),
    rangeInput,   // ← not encoded, unlike `query`
    endInput)

rangeInput values like "1h30m0s" don't contain URL-special characters, so this is safe in practice. But for consistency and future-proofing, wrapping in neturl.QueryEscape would match the treatment of query. Fix this →

Minor: Duration.String() may include unexpected components

Go's time.Duration.String() after Round(time.Second) produces strings like "1h30m0s", "2h0m0s". VMUI's g0.range_input typically expects formats like "1h", "30m", "90m". The "0s" and "0m" components may work depending on the VMUI version's duration parser, but this is worth verifying. If VMUI rejects compound durations, converting to minutes (e.g. fmt.Sprintf("%dm", int(d.Minutes()))) would be more robust. Fix this →

result.gotracesURL field placement

The comment and field are clear. No issues.

run_result.goTracesURL field position

TracesURL is placed between Spamoor and FieldDescriptions. Semantically it fits, and the omitempty tag is correct since tracing is optional. The fieldDescriptions entry uses the key "traces_url" (top-level) which matches the JSON output correctly. Good.

suite_test.gocollectTraces

Passing time.Now() at the moment of collection is the right call — this captures the end of the measurement window accurately.


No issues found with

  • Error handling (nil traceResult is never passed to buildRunResult given how newBenchmarkResult is used)
  • Security (no credentials, no user input in the URL path, service names are internal constants)
  • Concurrency (no shared mutable state introduced)
  • JSON schema (new field is additive and omitempty so existing consumers won't break)

Verdict

LGTM with the two minor nits above (URL encoding consistency, duration format). Neither is a blocker.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2026

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedApr 8, 2026, 10:38 AM

@chatton chatton marked this pull request as ready for review April 8, 2026 10:39
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 8, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.67%. Comparing base (536f57e) to head (3a15775).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3234   +/-   ##
=======================================
  Coverage   61.67%   61.67%           
=======================================
  Files         120      120           
  Lines       12635    12635           
=======================================
  Hits         7793     7793           
  Misses       3968     3968           
  Partials      874      874           
Flag Coverage Δ
combined 61.67% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@chatton chatton enabled auto-merge April 8, 2026 10:51
@chatton chatton added this pull request to the merge queue Apr 8, 2026
Merged via the queue into main with commit 6f09600 Apr 8, 2026
42 checks passed
@chatton chatton deleted the cian/fix-vm-ui-url branch April 8, 2026 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants