cubelog: emit the Region/Cluster defaults that Trace already computes

[dwin-gharibi]

Closes #1472.

Motivation

Trace() computed region, tmcluster and version fallbacks from the configured globals and then
never used them — staticcheck reports all three as SA4006. Because
makeLogFieldsFromTrace only sets Region/Cluster when the trace struct carries them, any caller
that omits them produced a trace line with no Region and no Cluster, even when the logger had defaults
configured.

Version was already defaulted inside makeLogFieldsFromTrace (entry.go:213-217), so the version
local was pure duplication.

What this changes

cubelog/metric.go in Trace():

• The computed region / tmcluster are now applied to the emitted fields.
• They are applied only when non-empty, which preserves the existing omit-when-empty contract —
  see below.
• The redundant version local is removed, since makeLogFieldsFromTrace already defaults it.

No comment changes.

The omit-when-empty detail

My first attempt set the fields unconditionally, which broke TestTraceEnds
(logger_test.go:92-93). That test asserts that when neither the trace nor the globals carry a
cluster, the output must not contain Cluster — an unconditional assignment made the key present
with an empty value.

The guarded version keeps that contract: absent stays absent, configured defaults now come through.
Both halves of TestTraceEnds pass.

Testing

New: cubelog/metric_trace_test.go

• TestTraceEmitsConfiguredRegionAndClusterDefaults — with globals configured and the trace fields
  empty, the emitted JSON carries the defaults.
• TestTracePrefersExplicitRegionAndCluster — an explicit value on the trace still wins over the
  global.

Both capture the real trace line via SetTraceOutput + EnableLogMetric and parse it as JSON, and both
restore the globals and the writer afterwards so they do not leak into other tests in the package.

Red/green verified — with metric.go reverted to master:

--- FAIL: TestTraceEmitsConfiguredRegionAndClusterDefaults
    metric_trace_test.go:49: Region = <nil>, want ap-guangzhou (the configured default)
    metric_trace_test.go:52: Cluster = <nil>, want cluster-a (the configured default)

and with the fix:

$ cd cubelog && go test -count=1 ./
ok  github.com/tencentcloud/CubeSandbox/cubelog  0.625s

$ go test -count=1 -run TestTrace ./          # the ordering that exposed the omit-when-empty issue
ok  github.com/tencentcloud/CubeSandbox/cubelog  0.164s

CI gates checked locally:

• gofmt -l . — clean (fmt-check).
• go build ./... — clean.
• staticcheck -checks 'SA4006' ./ — the three metric.go findings are gone.

Note the package directory is cubelog/ but the Go package is CubeLog; the new test file matches.

Risk / rollout

Trace lines from callers that omit Region/Cluster will start carrying the configured values. Anything
that parses those lines and currently treats the fields as absent will now see them populated — which
is the intended behaviour, but it is a visible change in log output.

Related, not fixed here

cubelog/metric.go:77-180 — the requestTrace / indexKey / realKey / slice2Str block is
unreachable (never instantiated). It looks like the other half of a partially-removed metric
aggregation path, and Trace() computing inputs for it is probably why these locals existed. Tracked
separately.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[cubesandboxbot]

Minor (non-blocking): region and tmcluster are now only consumed inside the enableLogMetric branch, but the fallback computation above still runs on every Trace() call — including the common case where metric logging is disabled (the package default at init). Since these values are read only here, moving the region/tmcluster computation inside this block would keep identical behavior and skip the wasted work when the feature is off. Not a correctness issue; the base code had the same unconditional computation, so this is a cleanup opportunity the PR naturally creates.

[cubesandboxbot]

Review: cubelog: emit the Region/Cluster defaults that Trace already computes

AI-generated review — not a human approval.

Summary

Small, correct, and well-tested change. Trace() now actually emits the Region/Cluster fallbacks it was already computing (fixing the SA4006 dead-store findings the PR describes), and the dead version local is removed since makeLogFieldsFromTrace already applies the moduleVersion default (entry.go:213-217).

I traced the value flow to confirm correctness:

• makeLogFieldsFromTrace only sets fields["Region"] / fields["Cluster"] when the trace struct carries a non-empty value (entry.go:232-240).
• The new assignments set those keys from the computed region / tmcluster, which equal the trace's explicit value when present and the configured global (defaultRegion / cluster) otherwise.
• Consequently: an explicit trace value wins (the re-assignment is idempotent), the key stays absent when no global is configured (preserving the TestTraceEnds omit-when-empty contract), and configured defaults now appear in the trace line. The non-empty guards are exactly right.

The version removal is safe: the local was never read after assignment in base metric.go:200-203, and the same moduleVersion default is applied inside makeLogFieldsFromTrace.

Findings

• Low / efficiency — cubelog/metric.go: region and tmcluster are computed on every Trace() call even when enableLogMetric is false (the package default at init). Since they are now only consumed inside that branch, the fallback computation could move inside the if enableLogMetric block with no behavior change. (Posted inline.)

Tests

The new cubelog/metric_trace_test.go is solid:

• It captures the real serialized trace line via SetTraceOutput + EnableLogMetric and parses it as JSON, rather than asserting on internal state.
• It covers both halves of the behavior: defaults emitted when the trace omits the fields, and explicit trace values winning over globals.
• Both tests restore defaultRegion, cluster, traceStd.writer, and enableLogMetric, so they don't leak into TestTraceEnds (which asserts on the omit-when-empty contract) or later tests. Given file ordering (logger_test.go runs before metric_trace_test.go), and the restore logic, TestTraceEnds stays green.
• The tests genuinely fail on the base code (no Region/Cluster keys when the trace omits them), matching the author's red/green claim.

Minor, non-blocking notes:

• captureTrace forces DisableLogMetric() in its defer rather than restoring the prior value. Fine today — TestTraceEnds re-enables it itself — but it couples the helper to test ordering more than restoring the saved value would.
• makeLogFieldsFromTrace is also used by the WithContext path (entry.go:104-105), so ordinary (non-Trace) log lines carrying a RequestTrace still drop the configured Region/Cluster defaults. If the underlying issue is broader than Trace() metric lines, that path remains inconsistent. This is out of scope for the PR as written, but worth tracking.

Risk

The PR's "Risk / rollout" section is accurate and appropriately flagged: trace lines from callers that omit Region/Cluster will now carry the configured values. This is the intended behavior change, and existing tests in the repo that consume Trace() output (e.g. CubeOps/internal/server/server_test.go) don't assert on these fields' presence, so no in-repo test breakage is expected.

Verdict

Approve. The change is minimal, correctly scoped to Trace(), matches the reported issue, and ships with passing coverage for both the new default-emission behavior and the preserved omit-when-empty contract.