[Geneva] Revert ETW payload framing that broke the agent - #4941
Merged
rajkumar-rangaraj merged 2 commits intoAug 5, 2026
Merged
rajkumar-rangaraj merged 2 commits into
rajkumar-rangaraj merged 2 commits into
Conversation
PR open-telemetry#4729 added a `byte[] data` parameter to `InformationalEvent` so the runtime-generated ETW manifest would match the payload, allowing .NET `EventSource` consumers to subscribe. To satisfy that manifest, `SendEvent` started writing two `EventData` descriptors instead of one: before: [ raw payload ] after: [ 4-byte LE length ][ raw payload ] `WriteEventCore` concatenates descriptors into the ETW user-data blob, so every log and span emitted over `EtwSession=...` gained a 4-byte prefix. The agent reads that blob verbatim as the forward protocol frame, which must begin with the msgpack array marker `0x93`. It now saw a length byte instead and rejected the data with "Bad forward protocol format". This reverts the transport to the pre-open-telemetry#4729 behaviour: a single descriptor carrying the unframed payload, and a parameterless `InformationalEvent`. The manifest deliberately declares no fields, because .NET prepends a synthetic length to any declared field. Only logs and traces on Windows were affected. Metrics use a separate transport (`MetricWindowsEventTracingDataTransport`) that still writes one descriptor, and the Linux user_events and TLD paths do not go through `EtwDataTransport`. Adds two regression tests, both of which fail against the open-telemetry#4729 implementation: * `TraceEventManifestDeclaresNoPayloadTemplate` asserts `GenerateManifest` emits `<event value="100">` with no `template` attribute. * `SendEventWritesRawPayloadWithoutDeclaredFields` asserts the emitted event decodes to zero payload fields. The roundtrip test added by open-telemetry#4729 could not catch this: it used an in-process `EventListener`, which strips the synthetic length while decoding, so it passed while the bytes on the wire were wrong. `EtwCollection` is removed. It existed only to serialize that roundtrip test. The listener helper now stores events in a `ConcurrentQueue` because `OnEventWritten` may be called from any thread.
Pull request dashboard statusMerged · refreshed 2026-08-05 20:54 UTC Status above doesn't look right?
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4941 +/- ##
==========================================
- Coverage 77.60% 77.50% -0.10%
==========================================
Files 468 468
Lines 19889 19887 -2
==========================================
- Hits 15435 15414 -21
- Misses 4454 4473 +19
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
martincostello
approved these changes
Aug 5, 2026
xiang17
approved these changes
Aug 5, 2026
This was referenced Aug 21, 2026
Closed
Closed
Merged
Merged
Open
This was referenced Sep 17, 2026
Closed
Open
Open
Open
Open
Closed
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #
Design discussion issue #
Changes
Reverts the
EtwDataTransportchanges from #4729, which altered the bytes written to ETW and caused the agent to drop all logs and spans emitted overEtwSession=...withBad forward protocol format.What broke
#4729 added a
byte[] dataparameter toInformationalEventso that the runtime-generated ETW manifest would match the payload, allowing .NETEventSourceconsumers to subscribe to the provider. In the ETW manifest format, .NET always prepends a synthetic 4-byte length to a declaredbyte[]field, soSendEventwas changed to write twoEventDatadescriptors instead of one:WriteEventCoreconcatenates descriptors into the ETW user-data blob, so every record gained a 4-byte prefix.The agent reads that blob verbatim as the forward protocol frame, which must begin with the msgpack array marker
0x93. With the prefix in place it saw a length byte instead and rejected the record.The byte layout is not just inferred from the
WriteEventCorecontract. Running the new tests against the #4729 implementation shows .NET's own manifest decoder reconstructing[1, 2, 3, 4]from the event, which is only possible if the blob on the wire was[4-byte length][payload].Blast radius
EtwSession=...(MsgPackLogExporter,MsgPackTraceExporter)MetricWindowsEventTracingDataTransport)EventProvider)1.16.0is unaffected. The diff betweenExporter.Geneva-1.16.0andExporter.Geneva-1.17.0contains exactly one functional file,EtwDataTransport.cs, plus a core version bump.The fix
Restores the pre-#4729 behaviour: a single descriptor carrying the unframed payload, and a parameterless
InformationalEvent. The manifest deliberately declares no fields, and there is now a comment on the event saying why, so this is not "fixed" again by accident.The only deltas from the pre-#4729 file are the expression-bodied
Send/IsEnabledintroduced later by #4849, which are unrelated and preserved, and that comment. The#pragma warning disable IDE0060added by #4849 is dropped, since it existed only to silence the unused parameter this PR removes.Tests
Two regression tests, both of which fail against the #4729 implementation:
TraceEventManifestDeclaresNoPayloadTemplateassertsEventSource.GenerateManifestemits<event value="100">with notemplateattribute. The manifest is what native consumers read, and a declared field is exactly what triggers the synthetic length.SendEventWritesRawPayloadWithoutDeclaredFieldsasserts the emitted event decodes to zero payload fields.Verified by checking out the #4729 implementation and running them:
The roundtrip test added by #4729 could not catch this. It used an in-process
EventListener, which strips the synthetic length while decoding, so it passed while the bytes on the wire were wrong. Raw ETW bytes are not observable in-process, which is why these tests assert the manifest and the decoded field count instead.EtwCollectionis removed, as it existed only to serialize that roundtrip test. Neither new test needs a real ETW session. Doing so surfaced a latent race in the listener helper, which stored events in a plainList<T>whileOnEventWrittencan be called from any thread, so it now uses aConcurrentQueue.Full suite passes on all eight target frameworks (net10.0, net9.0, net8.0, net48, net472, net471, net47, net462).
Trade-off
This deliberately reinstates the manifest/payload arity mismatch that #4729 set out to remove, so in-process
EventListenerand TraceEvent consumers still cannot decode these events. That capability and the agent's raw-blob contract are mutually exclusive on event ID 100. If .NET-side subscription is still wanted, it needs a separate event ID rather than a change to the existing one.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changesAddendum: corroboration from consumer-side diagnostics
Added after merge. Rejection diagnostics from the receiving agent confirm the mechanism above and rule out the alternatives.
The agent's rejection reports the msgpack type it decoded for the top-level object, and that type differs from record to record, including between records rejected in the same instant. A constant fault (wrong encoding, wrong table, truncation) would report the same type every time. A type that varies per record means the parser is reading a byte that varies per record.
That follows directly from the framing. Both exporters cap the buffer at
BUFFER_SIZE = 65360, so the length always fits in two bytes and the prefix is always:Byte 0, the only byte the parser inspects before failing, is the low byte of that record's serialized length. Mapping it onto msgpack type codes:
0x00-0x7F0xA0-0xBF,0xD9-0xDB0xE0-0xFF0x80-0x8F,0xDE,0xDF0xC4-0xC60x90-0x9F,0xDC,0xDDObserved rejections span several of these categories, consistent with the distribution above.
Notably, array is never among the rejected types. Those ~7% of records do not fail the array check, so the parser proceeds into the remaining bytes as though they were a valid frame. This is the more damaging branch: for a first byte of
0xDCor0xDDthe parser reads an array16/array32 header and takes its element count from the following bytes, which are[(size >> 8) & 0xFF][0x00][0x00][0x93]. For a ~22 KB record that decodes to an element count in the billions, so a single malformed record can drive an unbounded allocation in the consumer. At0xDDalone that is roughly 1 record in 256.Two consequences worth recording:
SendEventis unconditional, so on an affected build every record is malformed. Partial impact observed in the field comes from configuration and version splits, not from sampling:EtwSession=...combined withPrivatePreviewEnableTraceLoggingDynamic=trueresolves toTransportProtocol.EtwTldand never reachesEtwDataTransport, and metrics, Linux transports, and services still on1.16.0or earlier are untouched.