Skip to content

feat(internal/log/telemetry): add SDK error-reporting API for Error Tracking#4997

Draft
darccio wants to merge 12 commits into
mainfrom
dario.castane/hungry-einstein-959818
Draft

feat(internal/log/telemetry): add SDK error-reporting API for Error Tracking#4997
darccio wants to merge 12 commits into
mainfrom
dario.castane/hungry-einstein-959818

Conversation

@darccio

@darccio darccio commented Jul 8, 2026

Copy link
Copy Markdown
Member

Summary

  • Auto-forward sink: hooks internal/log.Error so every non-rate-limited call is mirrored to instrumentation telemetry. The format string is the constant dedup key; variadic error args are scrubbed via NewSafeError; a redacted stack trace is always attached. No change to local log output.
  • Policy table (internal/telemetry/log/policy.go): single place to classify format templates as report / downgrade (→ warn) / exclude. Pre-seeded with agent-connectivity and user-misconfiguration noise.
  • Explicit helpers (internal/telemetry/log/helpers.go): ReportError(msg, err, opts...) for swallowed errors; ReportPanic(recovered, msg) for recover() sites. Both enforce the constant-message contract.
  • constantlogmsg analyzer (internal/telemetry/log/analyzer/): go vet-compatible pass that rejects non-constant message arguments. Standalone binary for go vet -vettool; make lint/errlog target added.

Test plan

  • go build ./... — no compile errors
  • go vet ./internal/log/... ./internal/telemetry/log/... — clean
  • go test ./internal/log/... ./internal/telemetry/log/... -count=1 -race — all green (17 new tests)
  • make lint/errlog — no violations in existing codebase
  • No import cycle introduced (internal/log does not import internal/telemetry/log)
  • Local log output unchanged (verified by TestErrorLocalOutputUnchanged)

Reviewer notes

  • The sink is registered via init() in forward.go; it is active whenever any code imports internal/telemetry/log. Existing uses of log.Error require no changes — the auto-forward handles them. Non-constant first arguments will be flagged by the new analyzer.
  • The policyTable in policy.go is intentionally sparse. Prompt 2 (migration) will populate it as call sites are classified.
  • The analyzer uses fake package paths in testdata (bypasses the internal import restriction) and a New([]FuncSpec) constructor for test-scoped configuration.

Related epic: APMLP-1503

…racking

Introduce a centralized, PII-safe pipeline that forwards SDK-originated
errors from `internal/log.Error` to instrumentation telemetry (→ Error
Tracking) without changing existing local log output.

## What changed

**Auto-forward sink** (`internal/log/log.go`):
- `SetErrorTelemetrySink(f)` installs a forwarding hook; called before
  the local aggregation lock so telemetry is notified on every
  non-rate-limited `Error` call.
- `atomic.Pointer` ensures lock-free concurrent access.

**Sink registration** (`internal/telemetry/log/forward.go`):
- `init()` registers `forwardError` with `log.SetErrorTelemetrySink`.
- Uses the raw format string as the constant telemetry message (never
  interpolated); type-switches args to attach only `error` values, each
  scrubbed through `NewSafeError`.
- Always attaches a redacted stacktrace via `WithStacktrace()`.

**Policy table** (`internal/telemetry/log/policy.go`):
- Single source of truth for per-template actions: `report` (default),
  `downgrade` (→ warn), or `exclude`.
- Pre-seeded with agent-connectivity and user-misconfiguration templates.

**Explicit helpers** (`internal/telemetry/log/helpers.go`):
- `ReportError(msg, err, opts...)` — for swallowed-error branches.
- `ReportPanic(recovered, msg)` — for `recover()` sites.
- Both enforce the constant-message contract and policy table.

**`constantlogmsg` analyzer** (`internal/telemetry/log/analyzer/`):
- `go/analysis` pass that rejects non-constant first arguments on
  `log.Error`, `log.Warn`, `ReportError`, and `ReportPanic`.
- Standalone `cmd/main.go` runnable as `go vet -vettool`.
- Makefile target `make lint/errlog`.

**`MIGRATION-CONTRACT.md`**: canonical before/after for each of the
five call-site patterns Prompt 2 (automated migration) will encounter.

No existing log output is changed. No import cycle is introduced.
All tests pass under `-race`.
@datadog-prod-us1-6

datadog-prod-us1-6 Bot commented Jul 8, 2026

Copy link
Copy Markdown

Tests

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
Patch Coverage: 78.62%
Overall Coverage: 62.87% (+11.49%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: e9d5965 | Docs | Datadog PR Page | Give us feedback!

@pr-commenter

pr-commenter Bot commented Jul 8, 2026

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-07-10 15:51:12

Comparing candidate commit e9d5965 in PR branch dario.castane/hungry-einstein-959818 with baseline commit 1d87e06 in branch main.

Found 2 performance improvements and 5 performance regressions! Performance is the same for 318 metrics, 1 unstable metrics, 1 flaky benchmarks without significant changes.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

  • 🟩 = significantly better candidate vs. baseline
  • 🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

scenario:BenchmarkMetrics/distribution/get-handle

  • 🟥 execution_time [+3.183ns; +4.177ns] or [+2.538%; +3.331%]

scenario:BenchmarkOTLPProtoSize/1000spans

  • 🟥 execution_time [+21.039µs; +23.526µs] or [+6.660%; +7.447%]

scenario:BenchmarkOTLPProtoSize/100spans

  • 🟥 execution_time [+2.223µs; +2.447µs] or [+7.045%; +7.755%]

scenario:BenchmarkOTLPProtoSize/10spans

  • 🟥 execution_time [+220.497ns; +251.103ns] or [+6.828%; +7.775%]

scenario:BenchmarkOTLPProtoSize/1span

  • 🟥 execution_time [+26.878ns; +30.542ns] or [+6.917%; +7.860%]

scenario:BenchmarkPayloadVersions/simple_1000spans/v1.0

  • 🟩 allocated_mem [-1 bytes; -1 bytes] or [-2.857%; -2.857%]

scenario:BenchmarkPayloadVersions/simple_100spans/v1.0

  • 🟩 execution_time [-391.011ns; -345.589ns] or [-2.483%; -2.194%]

Known flaky benchmarks

These benchmarks are marked as flaky and will not trigger a failure. Modify FLAKY_BENCHMARKS_REGEX to control which benchmarks are marked as flaky.

Known flaky benchmarks without significant changes:

  • scenario:BenchmarkOTLPTraceWriterFlush

@kakkoyun

kakkoyun commented Jul 9, 2026

Copy link
Copy Markdown
Member

We should remove these files and make the new analyzer includes the functionality https://github.com/DataDog/dd-trace-go/blob/main/rules/telemetry_rules.go and https://github.com/DataDog/dd-trace-go/blob/main/rules/logging_rules.go

darccio added 4 commits July 9, 2026 17:16
… telemetrylog.{Debug,Warn,Error}

Extends DefaultFuncs to check all three telemetry log functions (and their
Logger method counterparts) in addition to the existing ReportError/ReportPanic
helpers. A single FuncSpec per name covers both call styles because resolveFunc
maps both pkg-level and method calls to the same (pkgPath, name) key via the
type checker.

Adds a skipPkgs mechanism (DefaultSkipPkgs, New variadic param) so the analyzer
does not flag the internal/telemetry/log package's own delegation calls — e.g.
func Error(message string,...){ defaultLogger.Load().Error(message,...) }.

Adds TestAnalyzerSkip to verify that listed packages are silently ignored even
when they contain variable-message calls.

This brings constantlogmsg to feature parity with the telemetryLogConstantMessage
ruleguard rule in rules/telemetry_rules.go, making it possible to drop that rule
once this analyzer is wired into all CI paths.
- gofmt the policy table (was failing the golangci-lint gofmt/goimports checks)
- wire the constantlogmsg analyzer into CI (static-checks.yml) and fix the
  violations it actually catches: a non-constant test call in log_test.go,
  and instrumentation.Logger's legitimate variable-message passthrough
  (added to DefaultSkipPkgs, same rationale as internal/telemetry/log itself)
- seed the policy table with the two named exclusion categories that were
  missing but already firing at Error level: W3C/propagation parse failures
  (propagating_tags.go) and sampler-rule errors (samplingrules.go)
- exclude the telemetry writer's own panic-encoding messages to avoid a
  self-referential report-about-the-reporter loop back into the pipeline
  that just failed
- implement the missing Warn opt-in forwarding path (SetWarnTelemetrySink /
  forwardWarn / warnOptedIn) — Warn was documented as "off by default,
  opt-in per template" but had no hook at all
- add benchmarks for the sink and helper hot paths (BenchmarkErrorWithTelemetrySink,
  BenchmarkForwardError(_Excluded), BenchmarkReportError, BenchmarkReportPanic)
- fix MIGRATION-CONTRACT.md's canonical recover() pattern, which double-reported
  telemetry for a single panic (local log.Error auto-forwards the error arg,
  then ReportPanic reports it again)
- update CONTRIBUTING.md and README.md for the new make lint/errlog target
Content has been copied over to Prompt 2 directly; no longer needed as a
file in this repo.
…th go/analysis checks

Kemal's review feedback on #4997: consolidate rules/telemetry_rules.go and
rules/logging_rules.go (golangci-lint gocritic/ruleguard DSL rules) into the
new constantlogmsg analyzer, so the SDK logging safety checks run as one
standalone go vet tool (`make lint/errlog`) instead of split across two
mechanisms.

- telemetrysafety.go: replaces telemetryLogSmartSlogAny, telemetryLogStringErrorCall,
  and telemetryLogRawErrorUsage. A single LogValuer-implements check subsumes the
  first and third (a raw error never implements slog.LogValuer); the message
  distinguishes "wrap with NewSafeError" for error-typed values from the generic
  LogValuer guidance for everything else.
- formatverbs.go: replaces internalLogFormatVerbs, stdLogFormatVerbs, and their
  err.Error()-suggestion counterparts. Covers internal/log's four functions
  (ruleguard's original scope) plus the standard "log" package in the same
  file allow-list as .golangci.yml's depguard config.
- nolint.go: a narrow //nolint reader (same-line or line-above, matching
  golangci-lint's own convention). Needed because these checks move from a
  gocritic-based linter (nolint-aware) to a standalone go vet pass (which
  isn't) — without it, existing `//nolint:gocritic` exceptions this migration
  inherited would start failing.
- cmd/main.go now runs all three analyzers via multichecker.

Deliberately did not extend constantlogmsg's own scope to internal/log's
Debug/Info (ruleguard checked all four): neither reaches telemetry, so the
PII-to-Error-Tracking rationale doesn't apply, and doing so surfaced an
unrelated passthrough test helper outside this PR's scope.

Verified the new checks introduce no regressions: ran the combined tool
against the whole repo before wiring in CI, found and fixed the 4 real
"prefer err.Error()" hits it revealed (manual_api_ddtest.go, sql.go x3) which
predated this PR, and confirmed telemetrysafety finds zero raw-error/non-LogValuer
violations repo-wide (existing SafeError usage is already consistent everywhere).

rules/telemetry_rules.go and rules/logging_rules.go removed, along with the
gocritic ruleguard settings in .golangci.yml and the now-unused
github.com/quasilyte/go-ruleguard/dsl direct dependency (go mod tidy).
@darccio darccio added the AI Generated Largely based on code generated by an AI or LLM. This label is the same across all dd-trace-* repos label Jul 10, 2026
@darccio darccio changed the title feat(internal/log,telemetry): add SDK error-reporting API for Error Tracking feat(internal/log/telemetry): add SDK error-reporting API for Error Tracking Jul 10, 2026
darccio added 6 commits July 10, 2026 13:22
- internal/telemetry/log/analyzer/nolint.go: use strings.SplitSeq instead of
  strings.Split in a range loop (modernize/stringsseq).
- Add missing copyright headers to the 8 new analyzer testdata files.
- internal/stacktrace/contribs_generated.go: drop
  github.com/quasilyte/go-ruleguard/dsl, matching what CI's generate check
  computed after the dependency was dropped from go.mod.
- README.md, scripts/README.md: sync the lint/errlog description text with
  the Makefile (make docs output), matching what CI's check-docs computed.

check-title was fixed separately by editing the PR title (comma in the scope
broke the Conventional Commits regex).
The lint job's reviewdog/action-golangci-lint step installs its own Go
toolchain and pins GOROOT to whatever it resolves (go1.26.4 in the failing
run). The Setup Go and development tools step I'd added afterward restores a
_tools/bin cache keyed by a *different* "stable" Go resolution (go1.26.5),
which had been built by a separate job. GOROOT stayed pinned to 1.26.4 while
the restored cache's tool binaries were built by 1.26.5, so `go run` failed
compiling the standard library with:

  compile: version "go1.26.4" does not match go tool version "go1.26.5"

Every other job in this file (copyright, check-docs, check-modules, etc.)
avoids this by giving each check its own clean job with a single Setup Go
step — nothing else touches Go beforehand. lint/errlog now follows the same
pattern in its own `lint-errlog` job instead of sharing the `lint` job with
the two golangci-lint steps.
This reverts commit d08c59f1c9ed81bef39fca4bd7ab55d2f4bee3ab.

Keeping lint/errlog in the shared lint job per review feedback; the actual
fix for the toolchain mismatch is dropping check-latest: true from the
shared setup-go composite action (next commit).
The lint job's reviewdog/action-golangci-lint step installs its own Go
toolchain first, resolving a loose "1.x" spec from whatever's already
cached on the runner (go1.26.4). The shared setup-go composite action ran
afterward requesting "stable" with check-latest: true, which bypasses the
action's bundled version manifest and always queries upstream for the
newest release — landing on go1.26.5, a patch newer than the manifest (and
the runner's pre-cached Go) knew about. GOROOT ended up pinned to the first
install while later steps picked up build artifacts expecting the second,
so `go run` failed compiling the standard library with:

  compile: version "go1.26.4" does not match go tool version "go1.26.5"

Dropping check-latest makes "stable" resolve through the same static
manifest basis every other install on the runner already uses, so it lines
up with whatever's already set up instead of racing ahead of it. This lets
lint/errlog stay in the shared lint job instead of needing its own.
…einstein-959818

# Conflicts:
#	go.mod
#	go.sum
#	internal/stacktrace/contribs_generated.go
CI's setup-go with check-latest can leave GOROOT pointing at an older
Go patch than the go binary on PATH, causing go/packages stdlib
recompiles to fail with "compile: version X does not match go tool
version Y". Clearing GOROOT lets go self-detect the correct root
instead of trusting a stale env var.
@darccio darccio marked this pull request as ready for review July 10, 2026 15:20
@darccio darccio requested review from a team as code owners July 10, 2026 15:20
@darccio darccio marked this pull request as draft July 10, 2026 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI Generated Largely based on code generated by an AI or LLM. This label is the same across all dd-trace-* repos

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants