feat(aspire): surface exception detail in routed OTLP logs (#6347)#6348
Conversation
When an SUT logs an exception, the OTLP body is usually just the message template; the failure detail lives in the exception.* semantic-convention attributes the OTLP log exporter attaches (OpenTelemetry .NET 1.8.0+). OtlpLogParser now reads LogRecord.attributes (field 6) and extracts exception.type / exception.message / exception.stacktrace. ProcessLogs prints the exception block after the body line, so a failing test shows why it failed. Prefers the full stack trace; falls back to "type: message".
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| CodeStyle | 1 minor |
🟢 Metrics 22 complexity
Metric Results Complexity 22
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[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
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-recordwhile (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 matchesHasException'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
OtlpLogRecordconstruction elsewhere isn't broken, and there's no other consumer of the record that needed updating (checked — onlyOtlpReceiver.ProcessLogsconstructs/reads it). - Backward compatible: no behavior change when a record carries no exception, per the added test and the
HasExceptionguard inProcessLogs.
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.
- Drop OtlpLogRecord.HasException — derivable from FormatException(); ProcessLogs now calls FormatException() once instead of walking the fields twice. - Guard the field-6 attribute parse (ParseExceptionAttribute) so only exception.* values are materialised — avoids allocating discarded value strings for the many non-exception attributes a record can carry on the ingest hot path. - Print the exception line with the same [severity] prefix as the body line. - Consolidate the test builder: BuildLogsExportRequest gains optional severity + exception params, replacing the near-duplicate BuildLogsExportRequestWithException.
There was a problem hiding this comment.
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
HasExceptionremoved: the record struct no longer duplicates the "has an exception" check separately fromFormatException().FormatException()is now the single source of truth, andOtlpReceiver.ProcessLogsbranches onFormatException() is not nulldirectly — exactly the suggested fix, so the two can no longer silently diverge if a futureexception.*attribute is added.- Selective attribute parsing: field 6 (
LogRecord.attributes) now goes through a dedicatedParseExceptionAttributethat only materializes the value string when the key matches one of the threeexception.*keys, instead of fully parsing every attribute via the genericParseKeyValue. 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 viaParser_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.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. What I verified:
Non-blocking note: |
1 participant
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.stacktracelog attributes (LogRecord.attributes, protobuf field 6). The receiver previously skipped field 6 entirely.OtlpLogParsernow reads field 6 and extracts those three attributes into newOtlpLogRecordfields.FormatException()renders them, preferring the full stack trace (which in OTel .NET isException.ToString(), so it already subsumes type+message) and falling back totype: message.OtlpReceiver.ProcessLogsprints the exception block right after the body line when present:Notes
HasExceptionfalse → nothing extra printed).Tests
Added 3 parser tests to
OtlpReceiverIngestionTests(full-block extraction, no-exception, type+message-only fallback). FullOtlpReceiverIngestionTestssuite green (11/11).