bugsnag: strip JSON objects from grouping strings, normalize LauncherError#261
bugsnag: strip JSON objects from grouping strings, normalize LauncherError#261morgan-wowk wants to merge 1 commit into
Conversation
f7c56a6 to
cff8f3a
Compare
| # Matches any embedded JSON object or Python dict literal (starts with `{"` or `{'`). | ||
| # These are stripped from grouping strings because they contain highly variable | ||
| # runtime data (e.g. full Kubernetes pod specs) that would fragment error groups. | ||
| _JSON_OBJECT_PATTERN = re.compile(r"\{['\"].*", re.DOTALL) |
There was a problem hiding this comment.
The regex pattern \{['"].* is overly greedy and will match from the first occurrence of {" or {' all the way to the end of the string, not just the JSON object itself. This means any text after a JSON object will also be stripped.
Example:
# Input: "Failed with {'error': 'x'} on server X"
# Current behavior: "Failed with {...}" # "on server X" is lost
# Expected: "Failed with {...} on server X"This contradicts the description which says it strips "JSON objects" (implying discrete chunks), not "everything after the first JSON object".
The pattern should be non-greedy and attempt to match balanced braces, for example:
_JSON_OBJECT_PATTERN = re.compile(r"\{['\"][^}]*\}", re.DOTALL)Or use a non-greedy quantifier:
_JSON_OBJECT_PATTERN = re.compile(r"\{['\"].*?\}", re.DOTALL)Note: Even these simpler fixes won't handle nested braces correctly, but would be better than the current greedy match.
| _JSON_OBJECT_PATTERN = re.compile(r"\{['\"].*", re.DOTALL) | |
| _JSON_OBJECT_PATTERN = re.compile(r"\{['\"].*?\}", re.DOTALL) | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
🤖 Good catch on both points. The greedy behaviour is intentional — anything after an embedded dict/JSON literal in an error message is typically also runtime-variable, so stripping to end-of-string is the right call for grouping stability. The comment has been updated to document this explicitly.
The whitespace fix (\{\s*) has also been applied so { "key": ...} now matches correctly.
| # Matches any embedded JSON object or Python dict literal (starts with `{"` or `{'`). | ||
| # These are stripped from grouping strings because they contain highly variable | ||
| # runtime data (e.g. full Kubernetes pod specs) that would fragment error groups. | ||
| _JSON_OBJECT_PATTERN = re.compile(r"\{['\"].*", re.DOTALL) |
There was a problem hiding this comment.
The regex pattern requires no whitespace between { and the quote character. Valid JSON and Python dict reprs often have whitespace like { "key": ...} or { 'key': ...} which will not match this pattern.
For example:
# Won't match: "Failed: { 'apiVersion': 'v1' }" # space after {
# Will match: "Failed: {'apiVersion': 'v1'}" # no spaceFix by allowing optional whitespace:
_JSON_OBJECT_PATTERN = re.compile(r"\{\s*['\"].*", re.DOTALL)This is critical for Kubernetes pod specs which the description specifically mentions, as their serialization format may vary.
| _JSON_OBJECT_PATTERN = re.compile(r"\{['\"].*", re.DOTALL) | |
| _JSON_OBJECT_PATTERN = re.compile(r"\{\s*['\"].*", re.DOTALL) | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
🤖 Fixed — pattern is now \{\s*['"].* so optional whitespace after { is handled.
|
|
||
|
|
||
| def _strip_generic(*, message: str) -> str: | ||
| message = _JSON_OBJECT_PATTERN.sub("{...}", message) |
There was a problem hiding this comment.
This should literally replace the _JSON_OBJECT_PATTERN with {...} and not just ... correct?
There was a problem hiding this comment.
🤖 Yes — the pattern replaces the matched span with {...}, which includes the opening {. So the result is {...} not ....
Also improved the pattern to handle optional whitespace after { (e.g. { "key": ...}) and updated the comment to document that the greedy match intentionally strips everything from the first dict/JSON literal to end-of-string.
| return None | ||
| # Take only the verb phrase before the first colon to drop any embedded | ||
| # serialized data (e.g. the full Kubernetes pod spec appended after ": "). | ||
| head = str(exception).split(":", 1)[0].strip() |
There was a problem hiding this comment.
🤖 This is an AI-generated code review comment.
_normalize_launcher_error over-truncates and may be redundant.
head = str(exception).split(":", 1)[0].strip()
return f"LauncherError: {head}"Splitting on the first colon collapses meaningfully distinct failures into one group. All of these become LauncherError: Failed to create pod:
"Failed to create pod: connection timeout to API server""Failed to create pod: name 'task-x' already exists""Failed to create pod: insufficient memory in node pool""Failed to create pod: {full pod spec}"
These are different operational problems and should group separately.
With the new _JSON_OBJECT_PATTERN already in _strip_generic, the generic fallback would produce LauncherError: Failed to create pod: {...} — keeping the verb phrase, marking the JSON-bearing suffix, and still grouping the spec-variable case. The specialized normalizer may not be needed at all.
The comment also calls it "the verb phrase," but for "creating pod: spec invalid: missing field 'name'" the head is just "creating pod" and the actual diagnostic is dropped.
Fix: either (a) drop _normalize_launcher_error entirely and let the generic path handle it now that _JSON_OBJECT_PATTERN exists, or (b) strip only the trailing JSON-y data and keep the rest:
message = _JSON_OBJECT_PATTERN.sub("{...}", str(exception))
return f"LauncherError: {message.strip()}"There was a problem hiding this comment.
🤖 Agreed — went with option (b). _normalize_launcher_error now applies _JSON_OBJECT_PATTERN.sub to the full message and keeps all the diagnostic context before the JSON blob. Failed to create pod: {'spec': ...} becomes LauncherError: Failed to create pod: {...} and creating pod: spec invalid: missing field 'name' passes through unchanged. Tests updated to match.
| # Matches any embedded JSON object or Python dict literal (starts with `{"` or `{'`). | ||
| # These are stripped from grouping strings because they contain highly variable | ||
| # runtime data (e.g. full Kubernetes pod specs) that would fragment error groups. | ||
| _JSON_OBJECT_PATTERN = re.compile(r"\{['\"].*", re.DOTALL) |
There was a problem hiding this comment.
🤖 This is an AI-generated code review comment.
_JSON_OBJECT_PATTERN greedily consumes everything after the first {\"/{' to end-of-string — worth documenting.
_JSON_OBJECT_PATTERN = re.compile(r"\{['\"].*", re.DOTALL)With re.DOTALL and the greedy .*, this matches from the first {\"/{' to end-of-string. Trailing context after an embedded JSON literal is also dropped, so:
RuntimeError("config: {'a': 1} during step X") → RuntimeError: config: {...}
RuntimeError("config: {'a': 1} during step Y") → RuntimeError: config: {...}
These group together even though "step X" vs "step Y" is a meaningful distinguisher.
This is likely intentional (anything after a runtime-data dict in an error message is usually also variable), but the comment explains the why for the JSON itself, not the trailing-text trade-off. One line in the docstring would help the next reader reason about edge cases — something like "Note: trailing message text after the first {\"/{' is also stripped".
If the trade-off is not desired, scope the match more tightly with a non-greedy variant like \{['\"].*?\} (with caveats around nested braces).
There was a problem hiding this comment.
🤖 Updated the comment above _JSON_OBJECT_PATTERN to explicitly document that trailing text is also stripped (intentional — anything after a runtime dict is typically also variable).
| result = error_normalization.normalize_error_message(exception=exc) | ||
| assert result == "LauncherError: launch failed" | ||
|
|
||
|
|
There was a problem hiding this comment.
🤖 This is an AI-generated code review comment.
Consider adding a test for multi-colon LauncherError messages.
The three new tests cover JSON suffix, cause chain, and no-colon. None covers the case where there is real diagnostic info after the first colon — exactly the scenario where the verb-phrase truncation silently degrades grouping quality (see the other comment on _normalize_launcher_error).
Adding:
def test_multiple_colons_in_message(self):
exc = self._make_launcher_error("creating pod: spec invalid: missing field 'name'")
result = error_normalization.normalize_error_message(exception=exc)
assert result == "LauncherError: creating pod" # or whatever the desired behavior is…either pins the truncation as intentional or surfaces the regression so the policy gets revisited.
There was a problem hiding this comment.
🤖 Added test_multi_colon_diagnostic_preserved which asserts creating pod: spec invalid: missing field 'name' passes through unmodified, confirming that diagnostic colons are kept now that we no longer truncate at the first colon.
|
|
||
| def _normalize_launcher_error(*, exception: BaseException) -> str | None: | ||
| try: | ||
| from ..launchers.interfaces import LauncherError |
There was a problem hiding this comment.
Wondering if it would make sense to do the import globally (at the top of the file) and have a global bool to track if LauncherError is available?
There was a problem hiding this comment.
🤖 Good suggestion! Moved to a module-level try/except with a _LAUNCHER_ERROR_AVAILABLE bool flag, so the import is attempted once at startup rather than on every _normalize_launcher_error call. The function now checks the flag directly.
…Error
Any embedded JSON/dict literal (starting with `{"` or `{'`) is now
stripped from error messages before they are used as grouping keys.
This prevents Kubernetes pod specs and similar runtime blobs from
fragmenting what is structurally the same error into many groups.
A dedicated normalizer for LauncherError is also added: it drops
everything after the first colon (the serialized pod spec) so that
"Failed to create pod: <spec>" normalizes to just
"LauncherError: Failed to create pod".
cff8f3a to
11f018b
Compare

Any embedded JSON/dict literal (starting with
{"or{') is nowstripped from error messages before they are used as grouping keys.
This prevents Kubernetes pod specs and similar runtime blobs from
fragmenting what is structurally the same error into many groups.
A dedicated normalizer for
LauncherErroris also added: it dropseverything after the first colon (the serialized pod spec) so that
"Failed to create pod: <spec>"normalizes to just"LauncherError: Failed to create pod".