Repository navigation
feat: Decode nested structure properties - #84
Conversation
|
Not merging yet — CI is red on all five The implementation is not what I think is wrong. In the same run, on the same code path, the So TDH resolves a schema for Kernel-Process on the runner and decodes its properties, while Kernel-Processor-Power yields nothing at all. The ETL content is committed and identical everywhere, so the only variable is the machine: Note the other two tests in the class passed, but both pass vacuously against an empty property dict, so they are not evidence either way. What I would not do: turn these into skips when the schema is unavailable. That is how the ETL tests spent their whole existence green and inert, and #77 has only just finished undoing that. A test that quietly stops testing on the machine that matters is worse than no test. Better options, roughly in order of preference:
The code itself reads well, for whenever the test situation is sorted:
One question on Happy to merge once CI is green. |
|
CI caught something the local run could not: the fixture decoded on my machine and produced zero struct properties on every runner. The cause was the fixture, not the parser. It was a capture of Re-recorded from a TraceLogging provider defined inside the generator script. Those events embed their own schema, so the ETL decodes identically anywhere; that is a property of the format rather than something to hope for. Windows ships the C# compiler the script uses, so it still needs nothing beyond a stock machine, and the fixture drops 40 KB → 16 KB. Each event now carries two Writing that fixture surfaced a second, unrelated bug, fixed in the following commit: TDH returns a counted string's two-byte length as part of the value, so it was decoded as content. The new fixture's Local: |
Struct-typed properties were skipped outright, so their contents were unreachable from Python. On this machine 1512 properties across 904 providers are declared as structs, and a single Kernel-Processor-Power core-parking event carries ten of them, all previously absent from the property dict. TDH addresses a struct member with a chain of PROPERTY_DATA_DESCRIPTORs -- the struct with the index of the instance, then the member -- so `property_bytes` grows into `property_bytes_at`, taking the whole path. Struct values are read recursively into `EventValue::Struct`, and a struct that is itself an array into an `Array` of them. Recursion is capped by MAX_STRUCT_DEPTH: a schema whose members pointed back at an enclosing struct would otherwise not terminate. Two supporting changes fall out of this. `read_layout` now walks all `PropertyCount` properties rather than only the top-level ones, because struct members live past `TopLevelPropertyCount` and `structType` addresses them by their index into that same array. Only the top-level entries become keys of the returned map; the rest are reached through the struct that owns them. Keeping that vector positionally aligned with `EventPropertyInfoArray` also fixes #81. A property with no name used to be dropped from the vector, which shifted every later index, so `PropertyParamCount` resolved its count from the wrong sibling. Unnamed properties now occupy their slot as a placeholder that is skipped when values are read. The struct work needs this alignment to be correct at all, so it is not separable. Verified against tracerpt on live Kernel-Processor-Power events. For the same core-parking event tracerpt reports Group {Number 0, Affinity 0xFFFF} and OldPark/NewPark/NewSoftPark {Number 0, Affinity 0x50C0}, and those are exactly the values that now come through. For an array of structs -- HardwareTable, 64 elements of {LogicalProcessorIndex, WorkLoadClassIndex, PerformanceClass, EfficiencyClass} -- every element matches too, and unlike tracerpt, which flattens all 64 into one repeated list because XML cannot express the grouping, the elements stay grouped. Closes #81 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HhPcm483PrDmuQEUSALBFn
sample.etl cannot exercise the struct path: Microsoft-Windows-Kernel-Process declares no struct-typed property, so the feature would have had no automated coverage at all and would only ever have been checked by hand on a live session. Add nested_struct.etl, 40 KB of Kernel-Processor-Power core-parking events carrying seventeen distinct struct properties, plus the script that produces it. The Algorithm keyword (0x20) is what selects those events; wider masks add hundreds of KB of unrelated telemetry without adding a struct, which is what keeps the fixture the same order of size as sample.etl. The tests assert the members are reachable, that they do not also leak in beside their struct at the top level, that structs survive into to_dict(), and that nested values are Python-native recursively. Arrays of structs are not in the fixture. The events carrying them fire rarely enough that capturing one reliably would have meant a multi-megabyte file; that case is covered by the bounds tests in tdh.rs and was verified by hand against tracerpt. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HhPcm483PrDmuQEUSALBFn
CI failed on all five Python versions: the fixture produced its events, but
not one struct-valued property, so both struct assertions fired.
The cause is the fixture, not the parser. It was a capture of
Kernel-Processor-Power, a manifest provider, and decoding a manifest event
needs that manifest installed *and of a matching version*. It decoded on the
machine that recorded it and yielded nothing on the CI runner's different
Windows build. A fixture that only decodes where it was made is worse than no
fixture, because it looks like coverage.
Record from a TraceLogging provider defined in the generator script instead.
Such events embed their own schema, so the ETL decodes identically anywhere,
which is a property of the format rather than something to hope for. Windows
ships the C# compiler the script uses, so this still needs nothing beyond a
stock machine, and the fixture drops from 40 KB to 16 KB.
Each event now carries two { Number, Affinity } structs with known values, so
the tests can assert the contents outright rather than just their shape.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HhPcm483PrDmuQEUSALBFn
TDH returns the two-byte count as part of the value for the counted in-types,
so it was being decoded as content. The new fixture's `Label` holds
"core-state" and came back as "\u{14}core-state", the `\u{14}` being its own
20-byte length. The same was visible earlier in a TraceLogging capture, where
"hello-from-32bit" read as " hello-from-32bit" -- the leading space was the
32-byte count, quiet enough to miss.
Affects win:CountedString, win:CountedAnsiString, win:CountedBinary and their
MOF equivalents.
The prefix is only removed when it actually describes the rest of the buffer.
Stripping unconditionally would eat the first character of any value that TDH
had already stripped, and this way a provider or TDH version that behaves
differently loses nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HhPcm483PrDmuQEUSALBFn
f089361 to
036723e
Compare
* feat: Recover schema-less payloads and offer TDH's display strings The last two gaps #72 listed for the TDH parser. **Events TDH cannot describe.** `TdhGetEventInformation` has no schema for WPP events without a matching TMF, nor for manifest providers whose manifest is not installed on this machine. Those events came back with an empty property dict and nothing else: their payload was read and then dropped. There are no names to report it under, so it cannot become properties -- keep the bytes instead. `EtwEvent.raw_data` has existed all along but was never filled in, the comment in session.rs explaining that ferrisetw keeps its `user_buffer` private. The record handed to TDH carries the same bytes and this module already holds it, so read `UserData` from there. Only for events with no schema: carrying a second copy of every decoded event's data would cost memory for nothing. `set_wpp_tmf_search_path` passes TDH a TDH_CONTEXT_WPP_TMFSEARCHPATH so a caller holding .tmf files can have WPP events decoded properly rather than falling back to bytes. Unset by default, in which case TDH is passed no context at all, exactly as before. **TDH's display strings.** #72 asked whether typed values or `TdhFormatProperty`'s strings should be the default and whether the other should be available. Typed values stay the default -- the Parquet/Arrow exporters need them -- and the strings become an opt-in second map, `formatted_properties`, behind `set_property_formatting`. Off by default, so the normal path pays nothing. They earn their place by expressing what typed values cannot, because TDH applies the manifest's value maps and output types: DynamicAddress typed=1 formatted='dynamic ' Address typed=b'\x02\x00\x00\x00\xc0\xa82\x01...' formatted='192.168.50.1' QueryBlob typed=2525199111760 formatted='0x24BF1989650' `TdhFormatProperty` is given one property's bytes, already isolated by `TdhGetProperty`, rather than the event payload with running offsets. That walk is what the rest of this module avoids on purpose, and skipping it costs nothing because TDH has already located the value. Arrays and structs are left out of the string view: `TdhFormatProperty` renders a single value, so handing it an array would silently describe only the first element -- a 176-byte TimeZoneInformation rendered as "228". `properties` reports those in full. The TMF search path is not verified end to end. This machine has no .tmf files and none of the WPP tooling that produces them. What is checked is that the context reaches TDH, that setting a path leaves all other decoding byte-identical, and that a path matching nothing is harmless. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HhPcm483PrDmuQEUSALBFn * test: Cover schema-less payloads, display strings, and the TMF path The existing fixtures cannot exercise the schema-less path: everything in sample.etl and nested_struct.etl decodes cleanly, which is the whole point of them. Add no_schema.etl -- the capture that used to be sample.etl before #76 replaced it. It was never a good property-parsing fixture, holding unrelated telemetry and no Kernel-Process events at all, but that is exactly what makes it the right one here: 73 of its 149 events have no schema TDH can find. Tests assert the payload survives for those, that decoded events do not also carry a redundant copy, that display strings appear only when switched on and leave the typed values untouched, and that a TMF search path matching nothing leaves all other decoding byte-identical. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HhPcm483PrDmuQEUSALBFn --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
Second stage of #72: struct-typed properties were skipped outright, so their contents were unreachable from Python. On this machine 1512 properties across 904 providers are declared as structs, and a single Kernel-Processor-Power core-parking event carries ten of them — all previously missing from the property dict.
Changes
EventValue::Struct. TDH addresses a struct member with a chain ofPROPERTY_DATA_DESCRIPTORs — the struct with the index of the instance, then the member — soproperty_bytesgrows intoproperty_bytes_at, taking the whole path. A struct that is itself an array becomes anArrayofStructs.MAX_STRUCT_DEPTH. A schema whose members pointed back at an enclosing struct would otherwise not terminate.read_layoutwalks allPropertyCountproperties, not just the top-level ones: struct members live pastTopLevelPropertyCount, andstructTypeaddresses them by index into that same array. Only top-level entries become keys of the returned map.EventPropertyInfoArrayis what the struct work needs to be correct at all, and it is exactly what Property index alignment breaks if a property has no name, mis-resolving PropertyParamCount #81 asks for. Unnamed properties now hold their slot as a placeholder rather than being dropped and shifting every later index.nested_struct.etl+ generator script, and four tests.Related Issue
Closes #81
Advances #72. Deliberately not closing it — see below.
Test Plan
Added/Modified Tests
tdh.rs: five new unit tests — depth guard,leaf_indexnesting, placeholder handling, and bothPropertyParamCountfallbacks (unnamed counter, out-of-range index). The last three pass a null record on purpose: reaching TDH would dereference it, so passing proves they short-circuit.test_etl_reader.py: four tests over the new fixture — members reachable, members not leaking to the top level, structs survivingto_dict(), nested values Python-native recursively.Test Results
Verified against tracerpt on live events rather than only by unit test. For one core-parking event tracerpt reports:
and this branch now produces, for the same event:
Arrays of structs,
HardwareTablewith 64 elements:against tracerpt's
0x33 0x38,0x3E 0x3C,0x5C 0x61,0x29 0x32— every element matches. Note tracerpt flattens all 64 into one repeated list because XML cannot express the grouping; these stay grouped, which is the more useful shape.Suite:
Checklist
Additional Notes
Why #72 stays open. Its proposed approach lists three more things this does not do, and I would rather they stay visible than be closed silently:
TdhGetEventInformationhas no schema to give — so it needs a different mechanism (TMF files for WPP), not more of this one. It is not rare: 73 of the 149 events in the old sample.etl had no resolvable schema.TdhFormatPropertystring output is not offered as an alternative to typed values. Still an open design question from the issue.The fixture is 40 KB, the same order as
sample.etl(32 KB). Keeping it there took picking theAlgorithmkeyword (0x20) specifically; wider keyword masks add hundreds of KB of unrelated telemetry without adding a single struct.One overlap to note:
git statuson this branch shows an untrackedsrc/pyetwkit/pyetwkit_core.pdb, left bymaturin develop. That is ignored by #83, not here.🤖 Generated with Claude Code
https://claude.ai/code/session_01HhPcm483PrDmuQEUSALBFn