-
Notifications
You must be signed in to change notification settings - Fork 20
bugsnag: chain-aware grouping key and chain title in error list #262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: graphite-base/262
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -104,7 +104,7 @@ def _normalize_launcher_error(*, exception: BaseException) -> str | None: | |
|
|
||
|
|
||
| def normalize_error_message(*, exception: BaseException) -> str: | ||
| """Return a stable normalized string for error grouping.""" | ||
| """Return a stable normalized string for a single exception (no chain traversal).""" | ||
| for normalizer in ( | ||
| _normalize_k8s_api_exception, | ||
| _normalize_max_retry_error, | ||
|
|
@@ -117,3 +117,46 @@ def normalize_error_message(*, exception: BaseException) -> str: | |
| return result | ||
|
|
||
| return f"{type(exception).__name__}: {_strip_generic(message=str(exception))}" | ||
|
|
||
|
|
||
| _CHAIN_PART_MAX_LEN = 80 | ||
|
|
||
|
|
||
| def normalize_error_chain(*, exception: BaseException) -> str: | ||
| """Return a stable normalized string covering the full exception chain. | ||
|
|
||
| Walks ``__cause__`` (and ``__context__`` when not suppressed) and joins | ||
| each level with `` <- ``. Use this for grouping keys so that chained | ||
| exceptions like ``LauncherError <- TimeoutError`` produce one stable group | ||
| rather than one per root cause. | ||
| """ | ||
| parts: list[str] = [] | ||
| seen: set[int] = set() | ||
| exc: BaseException | None = exception | ||
| while exc is not None and id(exc) not in seen and len(parts) < 4: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Grouping key produced by parts.append(normalize_error_message(exception=exc)) # no per-part capThis function is used as the Bugsnag custom grouping key. Each part has no length bound — only Fix: either (a) verify Bugsnag's actual custom-grouping-key length limit and document it in a comment, or (b) apply a generous per-part cap inside |
||
| seen.add(id(exc)) | ||
| parts.append(normalize_error_message(exception=exc)) | ||
| exc = exc.__cause__ or (None if exc.__suppress_context__ else exc.__context__) | ||
| return " <- ".join(parts) | ||
|
|
||
|
|
||
| def build_chain_title(*, exception: BaseException) -> str | None: | ||
| """Return a human-readable chain title for display, or ``None`` for single exceptions. | ||
|
|
||
| Like ``normalize_error_chain`` but truncates each level so the result fits | ||
| in a Bugsnag error list title. Returns ``None`` when there is no | ||
| chain so callers can skip overriding the default title. | ||
| """ | ||
| parts: list[str] = [] | ||
| seen: set[int] = set() | ||
| exc: BaseException | None = exception | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Duplicated chain-walking loop between Both functions share identical traversal logic — same Two places that have to stay in lockstep on every future change (max depth, Fix: extract one private helper: def _walk_chain(exception: BaseException, max_depth: int = 4) -> list[BaseException]:
excs: list[BaseException] = []
seen: set[int] = set()
exc: BaseException | None = exception
while exc is not None and id(exc) not in seen and len(excs) < max_depth:
seen.add(id(exc))
excs.append(exc)
exc = exc.__cause__ or (None if exc.__suppress_context__ else exc.__context__)
return excsThen |
||
| while exc is not None and id(exc) not in seen and len(parts) < 4: | ||
| seen.add(id(exc)) | ||
| part = normalize_error_message(exception=exc) | ||
| if len(part) > _CHAIN_PART_MAX_LEN: | ||
| part = part[:_CHAIN_PART_MAX_LEN].rstrip() + "..." | ||
| parts.append(part) | ||
| exc = exc.__cause__ or (None if exc.__suppress_context__ else exc.__context__) | ||
| if len(parts) <= 1: | ||
| return None | ||
| return " <- ".join(parts) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DEBUG-level logging hides SDK API drift in the title override.
The code is built on a load-bearing assumption that
event.errors[-1].error_classis the field Bugsnag displays as the error-list title. If a future SDK changes that shape, the assignment will throwAttributeError, the chain title silently disappears from Bugsnag, and the DEBUG log will be invisible in any production logging config. This is the kind of integration drift that doesn't break unit tests but quietly degrades the product.Fix: log at
warning(orerror) level. Still non-fatal — the original error still reports — but the next time the SDK shape drifts, you find out instead of guessing why the error list looks the way it used to.Note: the same DEBUG pattern at the prefix-prepending block above (~lines 55-59) has the same issue; worth bumping that too, although it's pre-existing.