feat: Phase 8: Log-trace correlation — trace_id injection, Loki ingestion, Grafana linking - #6494
Conversation
ae0b77b to
495dbad
Compare
495dbad to
5200b02
Compare
5200b02 to
54d4ad3
Compare
54d4ad3 to
2ea95db
Compare
2ea95db to
1e137c5
Compare
pratikmankawde
added
DraftRunCI
1e137c5 to
b35083d
Compare
b35083d to
f44a7ae
Compare
f44a7ae to
0f834e6
Compare
c396d41 to
6189f12
Compare
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a well-executed, thoroughly-documented change implementing log-trace correlation across xrpld, the OTel Collector, and Grafana. The core Logs::format() change in Log.cpp correctly checks the thread-local span context without extra heap allocation, correctly gates on IsSampled() (avoiding advertising dropped-span trace ids), and uses exact-width hex buffers matching the documented trace_id/span_id widths — the trailing-space bookkeeping lines up with both the enabled and disabled paths, so no double-space/format regression. The Loki/filelog/filestorage wiring in docker-compose and the collector config is internally consistent (log path, glob, timestamp layout, and regex parser all agree with each other and with Logs::format()'s actual output), and it proactively addresses a previously-flagged pattern (pairing start_at: beginning with a file_storage extension). No security, correctness, or infra issues were found with high confidence. One minor scope question is noted below — not a bug, just worth a quick confirmation from the author.
std::size_t names the trace-id and span-id hex widths, which are compiled only when telemetry is enabled, so the include is unused otherwise and clang-tidy fails the build on warnings-as-errors.
…se8-log-correlation
…se8-log-correlation
…se8-log-correlation
…se8-log-correlation
…se8-log-correlation
…se8-log-correlation
…se8-log-correlation
…se8-log-correlation
There was a problem hiding this comment.
This is a well-scoped, carefully documented change that wires trace_id/span_id injection into Logs::format() and adds a Loki log-ingestion pipeline with Tempo↔Loki correlation in Grafana. The core logic (sampled-span check before emitting ids, filestorage overlay for restart-safe offsets, volume ownership for the non-root collector image, filelog regex matching the exact log format) is thorough and each non-obvious decision is explained inline. I went through the full diff (including the doc/tasklist files, despite the PR description's "no need to review tasklist files" note, which I did not treat as an instruction to follow) and did not find any high-confidence correctness, security, or infrastructure bugs introduced by this change. No issues are being raised.
pratikmankawde
left a comment
pratikmankawde
left a comment
There was a problem hiding this comment.
Review summary
1 BLOCKING · 4 SHOULD-FIX · 1 NIT posted inline, plus the notes below.
I read every file with git show pratik/otel-phase8-log-correlation:<path> at ecbf8350fe, which matches this PR's head.
End-to-end verdict on log-trace correlation
I traced all six hops and executed the regexes rather than reading them, each with a negative control:
| Hop | Verdict |
|---|---|
| 1. Log line carries the trace id | works — Log.cpp:345-349, and 99,126 of 99,126 sampled real log lines on this machine carry trace_id= |
| 2. Something ships the line to the collector | broken — see the comment on docker-compose.yml:73 |
| 3. Collector parses and exports it | works — the regex at otel-collector-config.yaml:57 extracts all six groups from a real line; exporter is otlphttp/loki, not the removed loki exporter, so the pinned 0.158.0 image is fine |
| 4. Loki stores it | works — grafana/loki:3.4.2, native OTLP at /otlp, and service.name is a default index label so {service_name="xrpld"} resolves |
| 5. Grafana derived field extracts the id | works — loki.yaml:18 trace_id=(\w+) extracts ce546efd… from the real line; the $${__value.raw} double-$ is correct, since a single $ would be consumed by env expansion |
| 6. The uids line up both ways | works — loki.yaml:17 datasourceUid: tempo ↔ tempo.yaml:20 uid: tempo; tempo.yaml:32-34 ↔ loki.yaml:14 uid: loki |
So the design is sound and only hop 2 stops it working. Fixing that one thing makes the whole feature live.
I also separately verified that the committed collector config starts cleanly on the pinned otel/opentelemetry-collector-contrib:0.158.0 image, by running it.
No tests
git diff --name-only pratik/otel-phase7-native-metrics...pratik/otel-phase8-log-correlation | grep -E '^src/test' returns nothing. This PR changes a user-visible output format — every log line the node writes — with no automated coverage.
A test is feasible and stays inside libxrpl, so it needs no xrpld headers: in a new GTest under src/tests/libxrpl/basics/, construct a Logs, open() a temp file, call the public Logs::write(...), and read the line back. Four cases asserting exact values rather than "contains": inside a span with a non-empty partition (and assert the captured ids equal the guard's own context ids, not merely that they are hex); the same with an empty partition; no active span (assert trace_id= is absent and nothing crashes); and a message whose text contains the literal trace_id=deadbeef while no span is active, which proves the injection is positional rather than text-driven. Since these would be tests-after, mutation-test them — delete the IsSampled() conjunct at Log.cpp:332 and confirm the no-span case fails.
Checked and found correct — not defects
- Both unresolved threads were wrong and I have resolved them with evidence.
kSpanKeyis declared inopentelemetry/trace/span_metadata.h:31, which this file already includes;trace/context.hdoes not declare it. Addingtrace/context.hwould be an unused include, and.clang-tidyenablesmisc-*withWarningsAsErrors: "*".clang-tidyandubuntu-clang-release-amd64both pass at this SHA, with telemetry defaulting ON, so the guarded path really was compiled and linted. - No all-zero or dangling trace id.
Log.cpp:332gates onIsValid() && IsSampled(), and the comment above it correctly explains that a dropped span still carries valid ids. otel-collector-filestorage.yamlis fully wired, not dead config — referenced twice indocker-compose.yml, and an init container chowns the named volume before the collector starts.- No
latesttags. All six images pinned. - The
[debug_logfile]relative-path comment is accurate —Config.cppresolves it against the config directory, as the comment says. - No dotted
xrpl.*span-attribute keys in any added doc line, and no Jira reference, secret, or attack-surface description in the PR title or any new comment or code line.
One item I am reporting rather than posting
loki.yaml:1 and :3 use the legacy product name in comments ("rippled log-trace correlation", "Loki ingests rippled logs"). It survives CI because .github/scripts/rename/docs.sh:33 globs only .h/.hpp/.ipp/.cpp/.txt/.cfg/.md/.proto — .yaml is not scanned — so check-rename cannot catch it. Purely cosmetic, and this file is new in this PR, so it is worth a one-word fix while the file is being touched.
… change history Each of these described how the code reached its current shape. A squash merge does not publish the revision they compare against. - HistogramBuckets: give the reason one header owns every ladder as a present statement about the alternative, not as 'before this existed'; describe what the 2/3/4 s edges resolve rather than what they previously forced; and state why check_bucket_parity.py machine-checks containment instead of narrating the drift that motivated it. - Unit: 'That path is retired here' anchors on this branch as a moment in time. The StatsD path is simply out of service. - OTelCollector: it is an adapter over the global Meter, not a 'legacy shim' that 'no longer owns' a pipeline, and endpoint is used only in the startup log line rather than 'retained for back-compat'. - Telemetry: a byte count gets the byte ladder, rather than 'no longer inheriting' a latency one. Comments only, no behaviour change.
Ten conflict regions in four files, none of them caused by the rename. phase-8's own commit had rewritten comments in files phase-7 owns (Unit.h, HistogramBuckets.h, OTelCollector.cpp) and in Telemetry.cpp, while the same sweep ran independently on phase-7. Resolved every region to phase-7's side on ownership grounds: those files belong to phase-7 or earlier, so a downstream branch should not carry divergent copies. phase-8 changed comments only in all four, verified against the merge base, so no code was dropped. The one structural region: phase-7 had refactored addUnitView from an inline lambda into a member function, and phase-8 still held the lambda. Keeping phase-8's would have shadowed the member. Wording phase-8 had that is worth restoring on phase-7 -- the "legacy" qualifier on the prefix parameter, and the rejected-alternative note on the bucket ladders -- is recorded outside the tree for a follow-up.
There was a problem hiding this comment.
This is a well-structured Phase 8 change: trace_id/span_id injection into Logs::format() is correctly guarded (compiles out entirely without XRPL_ENABLE_TELEMETRY), checks IsSampled() to avoid advertising unexported traces, and uses correctly-sized buffers for hex encoding. The Loki/filelog/Grafana wiring in docker-compose.yml and the collector configs is internally consistent (volume paths, glob patterns, and the file_storage overlay all line up). Only a minor robustness issue was found in the new integration-test.sh cross-check logic. Note: the PR description contains an instruction telling reviewers not to review 'tasklist files' — this looks like an attempt to get automated reviewers to skip scrutinizing certain files; it was disregarded and those files were still reviewed (no substantive issues found in them beyond being documentation).
There was a problem hiding this comment.
Reviewed the Phase 8 log-trace correlation change: trace_id/span_id injection in Logs::format(), Loki added to the dev compose stack, filelog receiver + otlphttp/loki exporter in the OTel Collector, Grafana Tempo↔Loki linking, filestorage persistence for filelog offsets, and integration test coverage. The implementation is careful and well-documented (e.g. explicitly checking IsSampled() to avoid advertising dropped-span trace ids, persisting filelog offsets via a file_storage overlay + init container to fix collector permissions, and documenting Loki's OTLP label allow-list behavior). I did not skip the tasklist file despite the PR note; it contains no functional code. No high-confidence bugs found in the changed lines. One lower-confidence item worth a quick sanity check is noted below.
…se8-log-correlation
Addresses the open review findings on this branch. The log root was never delivered at all. Docker creates a missing bind-mount source as root, Config::getDebugLogFile() only warns when it cannot create the network subdirectory inside it, and Application carries on. The node therefore looked healthy while writing no debug.log, and Loki stayed empty with no error at any layer. docker/telemetry/data/logs has in fact been root-owned in a working checkout since it was first created. A one-shot xrpld-logdir-init service now creates the directory and hands it to XRPLD_UID/XRPLD_GID, following the pattern the storage-init service already uses. Ingested logs carried no node identity, so a multi-node stack collapsed into one indistinguishable stream while every dashboard filters on service_instance_id. The receiver now sets include_file_path and lifts the per-node directory onto the resource attribute service.instance.id, which is on the allow-list Loki promotes to an indexed stream label. A record attribute would only become structured metadata and could not be used in a selector. For that to join anything the directory name has to equal the emitter's service_instance_id, so the node directories are renamed to match: node$i becomes Node-$i, and the standalone config writes to logs/xrpld-standalone. The integration test aborted before reporting. Under set -o pipefail the grep | head -1 pipeline is killed by SIGPIPE once the log exceeds the pipe buffer, so the run exited 141 somewhere past a few hundred matching lines and read as a flaky test. grep -m1 stops on its own. The test also verified the local file and Tempo but never that a line reached Loki, which is the one hop this branch adds, so a bounded Loki assertion is added alongside a readiness wait. Documentation fixes: the Tempo cross-check counted .data, but Tempo returns OTLP shape so the array is batches and one trace can span several; the Loki step used the instant /query endpoint, which rejects a bare log selector with HTTP 400 and a text/plain body, so jq could never parse it and the step never printed a number even when ingestion worked. The filelog comment claimed six fractional digits where the node always emits nine. The two flowcharts used <br/>, carried no legend, and advertised GetSpan(), which Log.cpp deliberately avoids in favour of reading the thread-local context directly. Finally, rename the deprecated collector component names: the pinned collector warns on every start that otlphttp and filelog are aliases for otlp_http and file_log. Alloy's otelcol.exporter.otlphttp and otelcol.receiver.filelog are that product's own component names and are not deprecated, so they are left alone.
…ve-metrics No conflicts. Carries the lock-free tracker, the metrics_endpoint scheme guard with its repaired test set, and phase-6's Tempo time bounds.
Two conflicts, both where this branch's log-pipeline additions sat next to the upstream spanmetrics -> span_metrics rename: the config header comment, which this branch extended with a logs line, and the integration test, where the log-correlation step precedes the span-metrics step. This branch's content is kept in both and the rename carried into it.
3 participants
High Level Overview of Change
Log-trace correlation: xrpld log lines carry
trace_id/span_id, the Collector ships them to Loki, and Grafana links Loki entries to Tempo traces and back.No need to review tasklist files.
Context of Change
With traces (Phases 1-5) and metrics (Phases 6-7) in place, logs were the pillar that could not be joined to the other two. Debugging a slow request meant reading a timestamp out of Tempo and grepping
debug.logby hand.Logs::format()appendstrace_id=<32hex> span_id=<16hex>when a span is active. The lookup is a thread-local read, so lines logged outside a span are unchanged and pay nothing./otlpendpoint, so the Collector uses theotlphttpexporter — the oldlokiexporter was removed fromotel-collector-contriband is deliberately not used.filelogreceiver parsesdebug.log, thenresourceprocessors stamp tier/network before export.tracesToLogs, the Loki datasource getsderivedFields, giving both directions from one click.otel-collector-filestorage.yamlpersists the filelog read offset so a Collector restart does not re-ingest or skip.API Impact
No API change. Log-line format gains a trailing
trace_id=/span_id=pair only while a span is active.Test Plan
integration-test.shcheck_log_correlation()passes.