[thomhurst]

Fixes #6347

Problem

When an SUT test fails, TUnit routes the correlated OTLP log record to the test's output as [resource] [ERROR] <body>. But <body> is usually just the log message template — the actual exception/stack trace is dropped, so the output rarely explains why the test failed.

Fix

The OpenTelemetry .NET OTLP log exporter (1.8.0+) attaches the exception as exception.type / exception.message / exception.stacktrace log attributes (LogRecord.attributes, protobuf field 6). The receiver previously skipped field 6 entirely.

• OtlpLogParser now reads field 6 and extracts those three attributes into new OtlpLogRecord fields. FormatException() renders them, preferring the full stack trace (which in OTel .NET is Exception.ToString(), so it already subsumes type+message) and falling back to type: message.
• OtlpReceiver.ProcessLogs prints the exception block right after the body line when present:

[resource] [ERROR] Could not say hello
[resource] System.InvalidOperationException: boom
   at Sut.Fail()

Notes

• No behaviour change when a record carries no exception (HasException false → nothing extra printed).
• Complements the AspireFixture post-mortem stderr dump (feat(aspire): dump resource error logs on test failure + warn on missing telemetry wiring #6323) and live log forwarding ([TUnit.Aspire] Forward Aspire resource logs into captured test output (opt-in) #6341), which cover a different path (raw resource output, not OTLP-correlated records).

Tests

Added 3 parser tests to OtlpReceiverIngestionTests (full-block extraction, no-exception, type+message-only fallback). Full OtlpReceiverIngestionTests suite green (11/11).

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 minor

Alerts:
⚠ 1 issue (≤ 0 issues of at least minor severity)

Results:
1 new issue

Category	Results
CodeStyle	1 minor

View in Codacy

🟢 Metrics 22 complexity

Metric	Results
Complexity	22

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Review

Verified locally: TUnit.OpenTelemetry builds clean, and the full OtlpReceiverIngestionTests suite (11/11, including the 3 new parser tests) passes on pr-6348.

Correctness

The protobuf parsing looks right:

• LogRecord.attributes (field 6) is repeated, and it's read inside the existing per-record while (reader.TryReadTag(...)) loop, so multiple attributes (and non-exception ones) are all correctly consumed/skipped without desyncing the reader.
• FormatException()'s fallback chain (stacktrace → type: message → type-only → message-only → null) is exercised by the new tests and matches HasException's "any field non-empty" condition, so the two never disagree today.
• The new fields default via optional positional params, so the existing 5-arg OtlpLogRecord construction elsewhere isn't broken, and there's no other consumer of the record that needed updating (checked — only OtlpReceiver.ProcessLogs constructs/reads it).
• Backward compatible: no behavior change when a record carries no exception, per the added test and the HasException guard in ProcessLogs.

One design suggestion (non-blocking)

OtlpLogRecord.HasException and FormatException() independently re-derive "does this record have exception info" from the same three fields:

public bool HasException =>
    !string.IsNullOrEmpty(ExceptionStackTrace) || !string.IsNullOrEmpty(ExceptionType) || !string.IsNullOrEmpty(ExceptionMessage);

public string? FormatException() { ... same three fields, different branching ... }

They happen to agree today, but that agreement is only implicit — if a future change adds a 4th exception attribute (e.g. exception.escaped) and someone updates one method but not the other, HasException and FormatException() is null will silently diverge. Consider dropping HasException and making FormatException() the single source of truth; OtlpReceiver.ProcessLogs would become:

if (record.FormatException() is { } exceptionText)
{
    testContext.Output.WriteLine($"{prefix}{exceptionText}");
}

This also removes the double evaluation of the three string.IsNullOrEmpty checks per routed record (once for HasException, again inside FormatException()). Would need the two tests currently asserting on HasException directly to switch to asserting on FormatException() non-null instead — small change, but worth doing before this pattern gets copied elsewhere.

Nit

Every log attribute (not just exception.*) now gets fully parsed (key + value) via ParseKeyValue/ParseAnyValueString in the field-6 branch, where previously non-exception attributes were cheaply skipped via default: reader.Skip(wireType). For records with many unrelated attributes this is slightly more work per record, though almost certainly negligible for typical log volumes — not worth blocking on.

Overall: solid, well-scoped, tests match the stated behavior in the PR description. Approving with the suggestion above as optional follow-up.

[claude]

Review

Re-reviewed after the refactor(aspire): simplify OTLP exception surfacing after review follow-up commit. Verified locally: TUnit.OpenTelemetry builds clean (Release), and the full OtlpReceiverIngestionTests suite passes (11/11, including the 3 new parser tests).

Previous feedback — both addressed

• HasException removed: the record struct no longer duplicates the "has an exception" check separately from FormatException(). FormatException() is now the single source of truth, and OtlpReceiver.ProcessLogs branches on FormatException() is not null directly — exactly the suggested fix, so the two can no longer silently diverge if a future exception.* attribute is added.
• Selective attribute parsing: field 6 (LogRecord.attributes) now goes through a dedicated ParseExceptionAttribute that only materializes the value string when the key matches one of the three exception.* keys, instead of fully parsing every attribute via the generic ParseKeyValue. This restores the "skip uninteresting attributes cheaply" behavior that existed before this PR.

One residual note (non-blocking, but worth being aware of)

ParseExceptionAttribute (OtlpLogParser.cs) assumes the key field (1) is read before the value field (2) within each KeyValue submessage:

else if (fieldNumber == 2 && wireType == WireType.LengthDelimited
    && key is "exception.type" or "exception.message" or "exception.stacktrace")
{
    return (key, ParseAnyValueString(reader.ReadEmbeddedMessage()));
}

If an encoder ever wrote value before key, the value would hit the else branch (skipped, since key is still "" at that point) and be silently dropped — FormatException() would then omit that attribute with no error or diagnostic. This is a real (if currently theoretical) gap: unlike ParseKeyValue a few lines below, which captures the value into a local regardless of field order, this optimized variant is order-dependent by construction.

The comment above it already documents this as a deliberate assumption ("holds for all known OTel encoders"), consistent with the same trade-off already made elsewhere in this file (ParseResourceLogs assumes Resource precedes ScopeLogs), so I don't think this needs to block the PR. If you want extra defense-in-depth cheaply, buffering the raw KeyValue submessage bytes and doing a second pass only when needed is possible, but adds complexity for a case that hasn't been observed in practice — reasonable to leave as-is and revisit only if a non-conforming encoder shows up.

Correctness

• Field 6 is repeated and read inside the existing per-record loop, so multiple attributes (exception and non-exception) are consumed without desyncing the reader.
• FormatException()'s fallback chain (stacktrace → type: message → type-only → message-only → null) matches the new tests and the no-exception case is a true no-op (confirmed via Parser_LogWithoutException_HasNoExceptionDetail).
• Backward compatible: new fields default via optional positional params, existing 5-arg construction sites (if any remained) aren't broken.

Overall: solid fix, and the follow-up commit cleanly resolved the two points from the prior round. Approving.

[claude]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

What I verified:

• Built TUnit.OpenTelemetry and ran the full OtlpReceiverIngestionTests suite (11/11 passed, including the 3 new tests).
• OtlpLogParser's new attributes (field 6) handling and ParseExceptionAttribute correctly extract exception.type / exception.message / exception.stacktrace, with sensible fallback precedence in FormatException() (stacktrace → type+message → type → message → null).
• The "key precedes value" field-ordering assumption in ParseExceptionAttribute is explicitly documented and mirrors the same assumption already made elsewhere in this parser (e.g. ParseResourceLogs), so it's consistent with the existing design rather than a new risk.
• No TUnit.Core.SourceGenerator/TUnit.Engine dual-mode concerns (this is receiver/output-formatting code, not metadata collection), no VSTest usage, no reflection needing AOT annotations.

Non-blocking note: ParseLogRecord's case 6 now runs ParseExceptionAttribute (embedded-message parse + key string allocation) for every attribute on a log record, not just the three exception ones — previously all attributes were skipped without decoding. For records with many non-exception attributes this adds some parsing overhead versus a raw skip. Given this only runs on the OTLP log ingestion path (not a documented hot path per CLAUDE.md), this is likely fine as-is, but worth a quick check if high-cardinality attribute sets are expected in practice.