Skip to content

fix(core): DSPX-4999 restore database and external-core tracing - #4122

Merged
jakedoublev merged 1 commit into
mainfrom
codex/dspx-4999-platform-tracing
Oct 1, 2026
Merged

jakedoublev merged 1 commit into
mainfrom
codex/dspx-4999-platform-tracing

Conversation

@strantalis

@strantalis strantalis commented Oct 1, 2026 •

Copy link
Copy Markdown
Member

Proposed Changes

Restore missing runtime tracing for DSPX-4999:

  • Attach otelpgx to the shared database pool so recorded requests include SQL and pool-acquisition spans. Bind parameters remain excluded.
  • Add the existing Connect tracing interceptor to external-core SDK connections, matching IPC and ERS.
  • Let tracer shutdown flush queued spans after its initialization context is canceled, retaining the five-second timeout.

Tracing remains controlled by server.trace.enabled. SQL timings include reading rows; they do not isolate database execution from result transfer.

Related: #3307 already proposes SQL tracing alongside broader authorization instrumentation. This PR overlaps its database portion and adds the external-core and shutdown fixes on current main. #3722 covers separate CLI tracing work.

Testing Instructions

Passed: make fmt, service build, race tests for tracing, pkg/db, and pkg/server, SDK README tests, diff-scoped lint, and git diff --check. The cancellation regression fails before the fix and passes after it.

Full make test and service-wide tests require the unavailable Keycloak/Docker/platform stack. Full make lint reports existing findings in unchanged code; a separate scoped vulnerability check flags the installed Go 1.26.3 standard library. No unrelated fixes included.

After deployment, verify SQL/pool spans under ListAttributes, trace continuity to an external core, and no export with tracing disabled.

Checklist

  • Added a regression test for shutdown after cancellation
  • Updated tracing documentation
  • Verified deployed SQL and external-core trace linkage

Summary by CodeRabbit

  • New Features
    • Tracing now covers incoming and outgoing RPCs, PostgreSQL queries, and connection-pool acquisition during recorded requests. Database spans include SQL statements but not parameter values; query duration includes reading result rows.
    • Tracing can be disabled with server.trace.enabled.
  • Bug Fixes
    • Traces are now flushed during shutdown even if the context used to initialize tracing has been canceled.

Signed-off-by: strantalis <strantalis@virtru.com>
@strantalis
strantalis requested review from a team as code owners October 1, 2026 03:36
@github-actions github-actions Bot added the size/s label Oct 1, 2026
@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
AGENTS.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 2b2ac9ef-fb41-42fc-976f-4006967b7d31

📥 Commits

Reviewing files that changed from the base of the PR and between d06936f and f1cdb04.

⛔ Files ignored due to path filters (1)
  • service/go.sum is excluded by !**/*.sum
📒 Files selected for processing (6)
  • docs/Configuring.md
  • service/go.mod
  • service/pkg/db/db.go
  • service/pkg/server/start.go
  • service/tracing/otel.go
  • service/tracing/otel_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The change adds PostgreSQL and external SDK client tracing, documents tracing coverage, and updates tracer shutdown so initialization-context cancellation does not cancel shutdown.

Changes

Tracing updates

Layer / File(s) Summary
Database and external SDK tracing
service/go.mod, service/pkg/db/db.go, service/pkg/server/start.go, docs/Configuring.md
PostgreSQL connections use an OpenTelemetry tracer when a client tracer is set. The external SDK client receives a Connect tracing interceptor. The documentation describes tracing coverage, captured SQL statements, query duration, and the effect of disabling server.trace.enabled.
Shutdown after context cancellation
service/tracing/otel.go, service/tracing/otel_test.go
Shutdown derives its timeout context with context.WithoutCancel. The new test cancels the initialization context, calls shutdown, and checks that the exported file contains the span name.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: jakedoublev

Merge Risk: ⚪ Minimal · up to f1cdb

This change restores database and external-core tracing and lets tracer shutdown flush spans after the initialization context is canceled. No concrete merge-blocking risk was found. Deployed trace linkage was not verified.

Security Architecture Review

Security architecture risk: 🔵 Low · up to f1cdb

Tracing expands the operational data sent to configured collectors or files. Existing export enablement, authentication, and transport choices remain, and shutdown now preserves flushing after cancellation. Exact database trace payloads and deployment access controls remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The incremental exposure follows the shared instrumented database pool and external SDK connection, rather than one endpoint. Enabled telemetry can reach the configured OTLP collector or local trace file. The evidence does not establish collector tenancy, downstream sharing, or deployment-wide exposure.

Trust Boundaries and Controls

  • observed — External SDK setup retains the configured endpoint and transport options. Startup still supplies client credentials separately, and SDK construction composes its credential interceptor with extra tracing options. The inspected call path therefore does not replace authentication with trace identity.
  • observed — Enabled tracing uses a global TraceContext and Baggage propagator. The reused client helper disables trace events, and its unary propagation test verifies a shared client/server trace ID. That test does not exercise external SDK setup or prove filtering of baggage and other sensitive metadata.

Resilience and Maintainability Implications

  • inferred — The shutdown change improves preservation of queued telemetry during cancellation without granting new application authority. Provider ownership, flush-before-close ordering, and cleanup after failure remain intact. Timeout and concurrent shutdown were not directly exercised by the added repository regression test.

Hardening Proposals

  • proposed — Before enabling the expanded tracing coverage in sensitive deployments, verify the pinned database tracer's SQL, parameter, and error payloads, review query construction for embedded sensitive literals, and align trace-file and collector access controls with the data they receive.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 4 files. (2 skipped: 2… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely identifies the main change: restoring database and external-core tracing for DSPX-4999.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 4 files. (2 skipped: 2 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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

A rabbit traced the database trail,
Then followed calls beyond the gate.
The spans stayed safe when contexts failed,
And flushed their names before too late.
It twitched its nose and hopped away.

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

@github-actions github-actions Bot added the docs Documentation label Oct 1, 2026
@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 140.367181ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 74.63371ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 275.135024ms
Throughput 363.46 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 37.711317592s
Average Latency 376.210756ms
Throughput 132.59 requests/second

@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

⚠️ Govulncheck found vulnerabilities ⚠️

The following modules have known vulnerabilities:

  • otdfctl
  • service
  • tests-bdd

See the workflow run for details.

@jakedoublev
jakedoublev enabled auto-merge October 1, 2026 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Documentation size/s

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants