refactor(trace): retire span recording pipeline by LeftHandCold · Pull Request #25841 · matrixorigin/matrixone

LeftHandCold · 2026-07-17T08:59:12Z

What type of PR is this?

Which issue(s) this PR fixes:

What this PR does / why we need it:

MO no longer consumes internal Span records, but every hot-path trace.Start still entered the Span recording implementation and queried global controlled-span state. Under concurrency, that query appeared prominently in mutex-delay profiles.

This PR retires the Span recording pipeline while preserving the observability data and trace-context contracts that MO still uses:

removes MOTracer, MOSpan, MOHungSpan, the Span batch processor/exporter, controlled-span maps, timers, profile hooks, and Span-only tests;
defaults disable-span to true in code, independent of deployment configuration files;
keeps the fully disabled path on the zero-allocation NoopTracer;
uses a lightweight NonRecordingTracer when observability is enabled, generating and propagating TraceID/SpanID without recording, profiling, queuing, or exporting Spans;
makes mo_ctl TraceSpan requests return the deterministic response span tracing is no longer supported;
retains legacy Span configuration fields and public APIs as parse/source-compatible no-ops.

This is deliberately not a deletion of MO observability:

StatementInfo export, statement status/error fields, and CU accounting are unchanged;
structured/raw logs and MOErrorHolder export remain enabled and retain per-request TraceID/SpanID correlation;
SpanContext, context helpers, and the 24-byte MORPC wire layout are retained;
existing span_info schema/view definitions remain for upgrade compatibility, but no new Span rows are recorded;
the shared statement/log/error collector is still stopped and flushed during service shutdown.

Preserved non-recording behavior

Three behaviors were independent of Span export and are preserved explicitly:

WithHungThreshold previously introduced real context deadlines. The 1-minute storage-usage query deadline and 30-minute frontend cleanup guard are now explicit context.WithTimeoutCause scopes, with the original moerr.CauseNewMOHungSpan cause and former cancellation lifetime.
trace.Generate previously created per-connection IDs. The non-recording tracer preserves root/child/new-root ID semantics and context propagation while ordinary NoopTracer.Start remains zero-allocation.
The four former mo_ctl controlled diagnostic kinds (statement, remoteFSOperation, localFSOperation, and tnRPCHandle) were disabled by default. They remain default-off and preserve the parent context; the removed runtime enable/disable state, mutex, and map are not restored.

Regression tests prove that enabled Generate returns non-zero, distinct root IDs; the context reaches frontend routines, structured log rows, and error rows; MORPC encode/decode preserves the IDs; formerly controlled diagnostic starts do not overwrite the parent log context; and the fully disabled/ordinary no-op paths remain no-op.

Compatibility and behavior change

This intentionally removes the diagnostic Span opt-in. disable-span=false, enable-span-profile, long-span-time, and enable-trace-debug are still accepted to avoid configuration breakage, but cannot restore Span recording or Span-triggered profiles.

StatementInfo retains its existing statement, transaction, session, error, and CU fields. No fake Span rows or placeholder IDs are injected. Logs and errors use real non-recording correlation IDs generated for their request context.

BVT regression closure

The failed Matrixone Utils CI / Coverage job on head cb5e3217ce was caused by this PR, not by an unrelated main change. The initial NonRecordingTracer applied every requested kind, so a formerly default-off diagnostic start could change a log row's span_kind to localFSOperation, remoteFSOperation, or tnRPCHandle. MOZapLog.FillRow persists that kind verbatim, while zz_statement_query_type/query_stmt.sql accepts only internal, statement, session, or remote, producing the observed expected 1, actual 0.

Commit 6dc3383ce0 restores the old default behavior with a private static four-kind filter. It does not broaden the BVT allowlist, restore controlled-span shared state, or affect StatementInfo/CU/log/error collection. A regression test follows a controlled LocalFS start through ReportZap and raw-log row filling and verifies that the parent internal kind is retained.

Performance evidence

A controlled ABBA comparison on the same machine, configuration, data, workload, and concurrency (128 clients, 10 measured iterations per side) measured:

baseline: 79,393 QPS
Span-disabled candidate: 81,463 QPS
delta: +2.61%
baseline IsMOCtledSpan mutex-delay attribution: 43.77% / 40.58%
candidate exact IsMOCtledSpan focus: no samples

These percentages are mutex blocking-delay attribution, not CPU percentages. The result proves removal of that contention path; it does not show that Span was the primary CPU bottleneck or guarantee a 2.61% gain for every workload. Physical deletion has the same no-recording hot-path intent as the measured disabled candidate, so no additional performance gain is claimed for deletion itself. The lightweight context generator adds only ID/context propagation and does not restore the removed locks or exporter pipeline.

Validation

Validated at final head 6dc3383ce0:

focused regression tests for non-recording ID propagation and controlled-kind raw-log compatibility;
normal CGo-aware tests: ./pkg/util/trace/..., ./pkg/fileservice, ./pkg/frontend, ./pkg/sql/compile, ./pkg/common/log, and ./pkg/common/morpc;
race tests: ./pkg/util/trace/..., ./pkg/fileservice, ./pkg/frontend, and ./pkg/common/morpc;
CGo-aware go vet for the affected dependency chain;
make build;
git diff --check;
first-principles correctness, compatibility, lifecycle, wait-for, and Q1-Q3 unhappy-path review;
built binary contains NonRecordingTracer/ID-generation symbols but no executable MOTracer, MOSpan, MOHungSpan, controlled-span-state, or Span batch-processor symbols.

Machine 10.222.1.50 validation of the retired recording runtime:

rebuilt and started with an explicit disable-span=false configuration; Span recording still could not be re-enabled;
statement count increased and successful/failed marker statements retained status, error code, and CU;
raw-log and error counts increased; the injected SQL error appeared in system.error_info;
span_info remained unchanged at 3 -> 3;
CN and TN TraceSpan control calls returned span tracing is no longer supported;
1,000,000 point selects at concurrency 128 completed successfully;
mutex-profile exact focus for IsMOCtledSpan, MOTracer, MOSpan, and MOHungSpan: no samples;
graceful shutdown flushed the last statement buffer and logged trace.shutdown.complete;
the pre-existing system MO process was not modified.

The final diff is limited to the trace runtime/configuration, its TraceSpan compatibility endpoint, explicit preservation of the two former hung-context deadlines, non-recording trace-context correlation, and preservation of the former default-off controlled kinds. SQL execution, storage, fileservice, StatementInfo, and CU calculation semantics are not changed.

qodo-code-review · 2026-07-17T09:02:16Z

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

LeftHandCold · 2026-07-17T10:02:45Z

Final scope expansion and self-review completed at 62c1c608ba.

The PR now retires the Span recording implementation instead of only disabling it by default. Review-relevant boundaries:

StatementInfo, status/error fields, CU, logs, and error export remain active.
SpanContext/public helpers/RPC layout and span_info schema remain compatible.
legacy Span configuration parses but is a no-op; TraceSpan control returns an explicit unsupported response.
shared telemetry collector shutdown/flush ownership was migrated out of the deleted Span processor and is covered by normal/canceled-context tests.
no SQL execution, frontend request, storage, fileservice, or CU calculation semantics changed.

Final validation passed locally (race, focused/dependent tests, vet, full build) and on machine 50 with explicit disable-span=false. The 50-machine run retained Statement/log/error/CU records, created no new Span rows, completed a 1,000,000-query concurrency-128 workload, showed no IsMOCtledSpan/Span implementation samples in the mutex profile, and shut down with a successful final telemetry flush.

Detailed final implementation, all three validation rounds, and before/after evidence: #25839 (comment)

gouhongshen

Blocking finding

WithHungThreshold is not only a Span-recording option; it currently provides a real cancellation deadline. Before this PR, MOTracer.Start applied the option and newMOHungSpan wrapped the returned context with context.WithTimeoutCause(..., threshold, moerr.CauseNewMOHungSpan).

There are production call sites that consume that returned context:

pkg/util/metric/mometric/cron_task.go:253-255 passes it to sqlExecutor().Query, relying on the 1-minute bound.
pkg/frontend/routine.go:268 later checks routineCtx.Done() at line 345, relying on the 30-minute hung-request guard.

After this change, NoopTracer.Start returns the original context and never applies WithHungThreshold, so both deadlines disappear. A hung storage-size query/request can therefore outlive the previous bound and change shutdown/cancellation behavior. Please preserve this timeout semantics with an explicit context-timeout helper or at the call sites before making the Span runtime a no-op.

Other reviewed areas (retained StatementInfo/log/error registration, schema compatibility, CN/TN TraceSpan response path, and collector shutdown ownership) did not reveal an additional blocker.

Validation: go test -race ./pkg/util/trace -count=1 and go vet ./pkg/util/trace pass. The broader affected-package test command was blocked by missing local CGo headers (xxhash.h, roaring.h, usearch.h).

LeftHandCold · 2026-07-18T09:23:25Z

@gouhongshen @XuPeng-SH The P1 finding is valid and fixed in 7748b5706e.

The fix preserves correlation without restoring the retired Span pipeline:

when tracing is fully disabled, the existing NoopTracer remains zero-allocation and Start/Generate remain no-op;
when observability is enabled, NonRecordingTracer.Start creates root/child/new-root TraceID/SpanID contexts, but does not create recorded Span data, profile, enqueue, export, or consult controlled-span state;
frontend.NewRoutine -> trace.Generate again installs a non-empty, per-connection context;
structured log extraction, error-row extraction, and MORPC encode/decode all preserve the generated IDs.

Added regression coverage for the four requested boundaries:

enabled Generate creates non-zero IDs;
independent roots receive distinct TraceID and SpanID values;
generated IDs survive frontend routine setup, log/error extraction, and MORPC encoding/decoding;
fully disabled providers and ordinary NoopTracer.Start retain no-op behavior.

The branch was rebased onto main@2d01285f8b and revalidated:

normal CGo-aware tests passed for all affected trace, metric, frontend, config, control, service, exporter, log, and MORPC packages;
race tests passed for ./pkg/util/trace/..., ./pkg/util/metric/mometric, ./pkg/frontend, and ./pkg/common/morpc;
CGo-aware go vet, make build, and git diff --check passed;
the built binary contains the lightweight context generator but no executable MOTracer, MOSpan, MOHungSpan, controlled-span-state, or Span batch-processor symbols.

I also re-reviewed the full diff for retained StatementInfo/log/error collection, collector shutdown ownership, deadline/cancellation behavior, wire/schema compatibility, and resource/wait lifecycle. No additional blocker was found. Please re-review the updated head.

XuPeng-SH

Re-reviewed the complete diff at 7748b5706e. The previous blockers are closed:

the former WithHungThreshold business side effects are now explicit 1-minute and 30-minute contexts at the two real consumers; the DDL-only profiling call did not propagate its returned context and is correctly removed;
enabled observability now uses a stateless non-recording tracer that preserves root/child/new-root IDs, frontend connection correlation, log/error extraction, and MORPC propagation; fully disabled tracing and NoopTracer.Start remain no-op/zero-allocation;
Span recording, profile hooks, controlled-span maps/locks, queue/export paths, and runtime re-enable behavior remain retired; StatementInfo, log/error export, CU, schema, and wire compatibility remain intact;
shared collector shutdown has explicit ownership and a bounded cancellation path.

Performance was checked independently because there are 73 production trace.Start call sites, including SQL/fileservice hot paths. On the same machine, a focused latest-head non-recording Start+End benchmark measured about 55 ns/op, 256 B/op, 3 allocs/op versus about 178 ns/op, 384 B/op, 3 allocs/op for the main-branch MOTracer Start+End path. The new path therefore does not increase allocation count and removes the global controlled-span mutex and recording/export work.

Fresh reviewer validation passed:

CGo-aware race tests: ./pkg/util/trace/...
CGo-aware race test: frontend generated routine context
CGo-aware race test: MORPC generated-context codec round trip
git diff --check

GitHub Ubuntu UT, arm64 SCA, pessimistic multi-CN BVT, proxy BVT, and standalone pessimistic BVT are green; coverage is still running and should remain a merge gate. No remaining code blocker found.

gouhongshen

Codex automated review

No correctness regressions found in the PR diff. Verified trace-context propagation, collector shutdown ownership, retained telemetry registration, timeout preservation, and compatibility paths. pkg/util/trace tests, vet, and build passed; CGo-dependent focused packages could not run because this checkout lacks cgo/libmo.dylib and third-party headers.

mergify · 2026-07-18T10:11:32Z

Merge Queue Status

✅ Entered queue — 2026-07-18 10:11 UTC · Rule: main · triggered by rule Automatic queue on approval for main
❌ Checks failed · in-place
🚫 Left the queue — 2026-07-18 10:36 UTC · at cb5e3217ce9cdc8b8a2e03cb703122fdb50829e3

This pull request spent 25 minutes 13 seconds in the queue, with no time running CI.

Waiting for

All conditions

Reason

The merge conditions cannot be satisfied due to failing checks

Failing checks:

Hint

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio queue comment.

Requeued — the merge queue status continues in this comment ↓.

LeftHandCold · 2026-07-19T03:30:05Z

BVT failure: root cause confirmed and fixed

The failure in zz_statement_query_type/query_stmt.sql is caused by this PR; it is not an unrelated main failure.

Evidence chain:

On main, the four mo_ctl diagnostic span kinds (statement, remoteFSOperation, localFSOperation, and tnRPCHandle) are all disabled by default. Their starts return a no-op span without replacing the parent context.
The initial NonRecordingTracer removed the old controlled-state check but applied every requested kind. Therefore a formerly disabled LocalFS/RemoteFS/TN-RPC start could replace the parent kind.
MOZapLog.FillRow writes SpanContext.Kind.String() directly to system.rawlog.span_kind.
The failing BVT accepts only internal, statement, session, or remote; a leaked diagnostic kind makes its expression return the observed 0.

Fixed in 6dc3383ce0 by keeping exactly those four retired controlled kinds default-off with a private static filter. This preserves the pre-PR default semantics without restoring the controlled-span mutex/map/runtime switch and without changing the BVT allowlist.

Regression coverage now verifies the complete failing path: an internal root context -> controlled LocalFS start -> ReportZap -> MOZapLog.FillRow, with the raw-log kind remaining internal. StatementInfo, CU, logs, errors, and non-recording trace correlation are unchanged.

Validation after the fix:

focused regression tests passed;
normal CGo-aware tests passed for trace, fileservice, frontend, SQL compile, log, and MORPC;
race tests passed for trace, fileservice, frontend, and MORPC;
affected-package go vet, make build, git diff --check, and the complete lifecycle/unhappy-path review passed.

Fresh CI is running at https://github.com/matrixorigin/matrixone/actions/runs/29671757688; the same Matrixone Utils CI / Coverage job is included.

mergify · 2026-07-19T04:21:43Z

LeftHandCold temporarily deployed to ci July 17, 2026 08:59 — with GitHub Actions Inactive

mergify Bot added kind/enhancement kind/test-ci labels Jul 17, 2026

LeftHandCold had a problem deploying to ci July 17, 2026 08:59 — with GitHub Actions Error

LeftHandCold temporarily deployed to ci July 17, 2026 08:59 — with GitHub Actions Inactive

LeftHandCold mentioned this pull request Jul 17, 2026

[Performance]: retire Span recording to eliminate IsMOCtledSpan contention #25839

Closed

matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Jul 17, 2026

LeftHandCold marked this pull request as ready for review July 17, 2026 09:02

LeftHandCold requested review from XuPeng-SH, fengttt and gouhongshen as code owners July 17, 2026 09:02

LeftHandCold force-pushed the agent/default-disable-span branch from 08ed69d to 62c1c60 Compare July 17, 2026 09:46

LeftHandCold requested a review from aunjgr as a code owner July 17, 2026 09:46

LeftHandCold temporarily deployed to ci July 17, 2026 09:46 — with GitHub Actions Inactive

LeftHandCold temporarily deployed to ci July 17, 2026 09:47 — with GitHub Actions Inactive

matrix-meow added size/XXL Denotes a PR that changes 2000+ lines and removed size/S Denotes a PR that changes [10,99] lines labels Jul 17, 2026

LeftHandCold changed the title ~~perf(trace): disable spans by default~~ refactor(trace): retire span recording pipeline Jul 17, 2026

mergify Bot added kind/refactor Code refactor kind/api-change labels Jul 17, 2026

gouhongshen requested changes Jul 17, 2026

View reviewed changes

fengttt approved these changes Jul 17, 2026

View reviewed changes

LeftHandCold requested review from XuPeng-SH and gouhongshen July 18, 2026 09:21

XuPeng-SH approved these changes Jul 18, 2026

View reviewed changes

gouhongshen approved these changes Jul 18, 2026

View reviewed changes

mergify Bot added the queued label Jul 18, 2026

Merge branch 'main' into agent/default-disable-span

cb5e321

mergify Bot temporarily deployed to ci July 18, 2026 10:11 Inactive

mergify Bot temporarily deployed to ci July 18, 2026 10:12 Inactive

mergify Bot had a problem deploying to ci July 18, 2026 10:12 Failure

mergify Bot added dequeued and removed queued labels Jul 18, 2026

mergify Bot had a problem deploying to ci July 18, 2026 16:14 Failure

fix(trace): keep retired controlled span kinds disabled

6dc3383

LeftHandCold temporarily deployed to ci July 19, 2026 03:26 — with GitHub Actions Inactive

mergify Bot removed the dequeued label Jul 19, 2026

mergify Bot added the kind/bug Something isn't working label Jul 19, 2026

Merge branch 'main' into agent/default-disable-span

bbf2d33

LeftHandCold mentioned this pull request Jul 19, 2026

[Bug]: caller timeout resets the shared HAKeeper client and cascades connection failures #25868

Closed

Navigation Menu

Uh oh!

refactor(trace): retire span recording pipeline - #25841

refactor(trace): retire span recording pipeline#25841
mergify[bot] merged 7 commits into
matrixorigin:mainfrom
LeftHandCold:agent/default-disable-span

LeftHandCold commented Jul 17, 2026 •

edited

Loading

Uh oh!

qodo-code-review Bot commented Jul 17, 2026

Uh oh!

LeftHandCold commented Jul 17, 2026

Uh oh!

gouhongshen left a comment

Uh oh!

LeftHandCold commented Jul 18, 2026

Uh oh!

XuPeng-SH left a comment

Uh oh!

gouhongshen left a comment

Uh oh!

mergify Bot commented Jul 18, 2026 •

edited

Loading

Uh oh!

LeftHandCold commented Jul 19, 2026

Uh oh!

mergify Bot commented Jul 19, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

LeftHandCold commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

Which issue(s) this PR fixes:

What this PR does / why we need it:

Preserved non-recording behavior

Compatibility and behavior change

BVT regression closure

Performance evidence

Validation

Uh oh!

qodo-code-review Bot commented Jul 17, 2026

Qodo reviews are paused for this user.

Uh oh!

LeftHandCold commented Jul 17, 2026

Uh oh!

gouhongshen left a comment

Choose a reason for hiding this comment

Blocking finding

Uh oh!

LeftHandCold commented Jul 18, 2026

Uh oh!

XuPeng-SH left a comment

Choose a reason for hiding this comment

Uh oh!

gouhongshen left a comment

Choose a reason for hiding this comment

Codex automated review

Uh oh!

mergify Bot commented Jul 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Reason

Hint

Uh oh!

LeftHandCold commented Jul 19, 2026

BVT failure: root cause confirmed and fixed

Uh oh!

mergify Bot commented Jul 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LeftHandCold commented Jul 17, 2026 •

edited

Loading

mergify Bot commented Jul 18, 2026 •

edited

Loading

mergify Bot commented Jul 19, 2026 •

edited

Loading