Skip to content

feat(telemetry): record root cause on WinMLCLIError#1111

Open
timenick wants to merge 6 commits into
mainfrom
zhiwang/telemetry-root-cause
Open

feat(telemetry): record root cause on WinMLCLIError#1111
timenick wants to merge 6 commits into
mainfrom
zhiwang/telemetry-root-cause

Conversation

@timenick

Copy link
Copy Markdown
Collaborator

Summary

  • Record the innermost root cause of an exception chain on WinMLCLIError, so wrapped errors (e.g. ClickException around a ValueError) become analyzable instead of collapsing to ErrorType=ClickException.
  • New attributes root_cause_type (for grouping) and root_cause_message (scrubbed, capped at 500 — larger than the outer 200 cap since the root cause is the diagnostic payload). Both are None when the outer exception has no deeper cause, so "no chain" is visually distinct from a real root cause.

Changes

  • telemetry/utils.py: new _root_cause chain-walker (prefers __cause__ over __context__, walks to innermost, cycle-safe via id()); _format_exception_message gains a cap param; new _ROOT_CAUSE_MESSAGE_CAP = 500.
  • telemetry/telemetry.py: log_error resolves the root cause and emits the two new attributes; both added to the WinMLCLIError allowlist.

Notes

Part of #1110

@timenick timenick marked this pull request as ready for review July 14, 2026 09:09
@timenick timenick requested a review from a team as a code owner July 14, 2026 09:09
Comment thread src/winml/modelkit/telemetry/utils.py Fixed
timenick added 2 commits July 14, 2026 17:16
…t-cause

# Conflicts:
#	tests/unit/telemetry/test_utils_scrubbing.py

@xieofxie xieofxie left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/winml/modelkit/telemetry/utils.py Outdated

@xieofxie xieofxie left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants