feat(telemetry): record root cause on WinMLCLIError#1111
Conversation
…fix CodeQL unused-global
…t-cause # Conflicts: # tests/unit/telemetry/test_utils_scrubbing.py
xieofxie
left a comment
There was a problem hiding this comment.
Nice, well-scoped Part-1 change. The chain-walker is clean — __cause__-over-__context__ preference, innermost walk, and id()-based cycle guard are all correct and thoroughly tested (I ran the full telemetry suite locally: 224 passed). The cap parameterization preserves the scrub-before-cap ordering, and has_root = root is not exc gives a clean null distinction for "no chain". Splitting the command-layer from-dropping fixes into Part 2 is the right call.
One behavioral nuance on _root_cause worth considering before merge (inline) — otherwise LGTM.
xieofxie
left a comment
There was a problem hiding this comment.
Re-reviewed at eae6f92a. The __suppress_context__ point is fixed exactly as suggested: _root_cause now only falls through to __context__ when it isn't suppressed, so raise ... from None correctly surfaces no chain — matching Python's own traceback behavior and the developer's intent to hide the inner error. The docstring is updated and test_root_cause_honors_suppressed_context locks it in. Ran the affected telemetry tests locally: 66 passed. LGTM.
Summary
WinMLCLIError, so wrapped errors (e.g.ClickExceptionaround aValueError) become analyzable instead of collapsing toErrorType=ClickException.root_cause_type(for grouping) androot_cause_message(scrubbed, capped at 500 — larger than the outer 200 cap since the root cause is the diagnostic payload). Both areNonewhen the outer exception has no deeper cause, so "no chain" is visually distinct from a real root cause.Changes
telemetry/utils.py: new_root_causechain-walker (prefers__cause__over__context__, walks to innermost, cycle-safe viaid());_format_exception_messagegains acapparam; new_ROOT_CAUSE_MESSAGE_CAP = 500.telemetry/telemetry.py:log_errorresolves the root cause and emits the two new attributes; both added to theWinMLCLIErrorallowlist.Notes
commands/quantize.py:327) is intentionally out of scope — the plan is to use the new telemetry data to prioritize the highest-frequency offenders.Part of #1110