fix(telemetry): emit error events at ERROR severity and declutter slash menu - #129
Merged
Conversation
Claude Code does not read AGENTS.md automatically. Add a CLAUDE.md that imports the tracked repo rules (AGENTS.md) and the machine-local overlay (AGENTS.local) so Claude Code sessions get the same guidance Pythinker injects via PYTHINKER_AGENTS_MD.
…tributes
track("error"/"crash"/"api_error") forwarded every event to OTel logs at the
emit_log() default of INFO, so error/crash telemetry was indistinguishable from
product analytics — SigNoz severity filters and the error saved views found
nothing, and the views' error_type filter never matched (call sites flatten it
to property.error_type / property.exc_class).
- Map known error event names to severity in EventSink: error/crash/api_error ->
ERROR, session_load_failed -> WARN; everything else stays INFO.
- Add canonical error.type/error.site/error.expected/error.kind attributes for
error-like events so dashboards query one set of keys regardless of whether
the call site emitted error_type or exc_class. Original property.* preserved.
- Factor OTel emission into emit_events_to_otel() and have flush_sync() drain the
pre-sink _event_queue, so a startup crash before attach_sink() still reaches
SigNoz (Bugsink already captured it).
- Docs: correct the stale claim that report_handled_error always calls Sentry;
expected errors are withheld from Bugsink.
Tests cover severity mapping, canonical attributes, and crash-safe flush.
Add a blank gap line between the input row and the slash command popup, drop the redundant [command]/[shell] tag (keeping the distinguishing [skill]/[flow] tags), and add a persistent footer legend set off by its own separator line. When the list overflows, the footer folds in a '+N more' count instead of silently hiding entries; the menu height adapts to the terminal and is capped to leave room for the chrome rows.
Contributor
|
Warning Review limit reached
More reviews will be available in 56 minutes and 32 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR enhances telemetry robustness and refines the slash command menu UI. The telemetry changes add crash-safe event flushing to OpenTelemetry when no sink is attached, with severity mapping and canonical error attribute enrichment. The UI changes clean up the completion menu by removing redundant scope tags, adding fixed gap/footer chrome with overflow indicators, and adapting heights to terminal constraints. ChangesTelemetry Crash-Safe Flush
Slash Command Menu UI Layout and Filtering
Documentation and Configuration
Sequence DiagramsequenceDiagram
participant flush_sync as flush_sync()
participant queue as Event Queue
participant sink as EventSink
participant emit_otel as emit_events_to_otel()
participant otel as otel.emit_log()
flush_sync->>sink: check if attached
alt No sink, queued events exist
flush_sync->>queue: backfill device_id/session_id
flush_sync->>emit_otel: emit_events_to_otel(events)
emit_otel->>emit_otel: map event name to severity
emit_otel->>emit_otel: enrich error.* attributes
emit_otel->>emit_otel: validate schemas
emit_otel->>otel: emit_log with severity
otel-->>emit_otel: ✓
flush_sync->>queue: clear
else Sink attached
flush_sync->>sink: _sink.flush_sync()
sink-->>flush_sync: ✓
else Queue empty
flush_sync-->>flush_sync: no-op
end
flush_sync->>flush_sync: sentry.flush / otel.shutdown
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pythinker_code/telemetry/__init__.py`:
- Around line 209-218: The blanket with suppress(Exception) around the
import/backfill/emit block should be replaced with explicit try/except so errors
aren’t silently swallowed: wrap the import of pythinker_code.telemetry.sink and
the loop that backfills device/session ids plus the call to
emit_events_to_otel(...) in a try block, catch Exception as e, log the exception
(at debug/error level) with context, and only clear _event_queue on successful
emit; also ensure you still perform the backfill of _device_id/_session_id
inside the try before calling emit_events_to_otel and do not lose the original
events on failure (leave the queue intact or persist as intended). Reference
symbols: suppress(Exception) (remove), emit_events_to_otel, _event_queue,
_device_id, _session_id.
In `@src/pythinker_code/telemetry/sink.py`:
- Around line 105-108: The try/except around "from pythinker_code.telemetry
import otel as _otel" silently swallows import failures; before returning, log
the exception with context (e.g., get a module logger via
logging.getLogger(__name__) or use the existing module logger) and emit a debug
or warning message including exc_info=True so the import/init error is recorded,
then return as before.
In `@tests/ui_and_conv/test_slash_completer.py`:
- Around line 407-408: Replace the confusable literal glyph ❯ in the assertions
with an explicit escaped code-point constant (e.g. RIGHT_ARROW = "\u276F" or use
the \N{name} escape) and use that constant in the two asserts that reference
rendered_lines (replace the literal in assert "❯" in rendered_lines[1] and
assert "❯" not in rendered_lines[2] with assert RIGHT_ARROW in rendered_lines[1]
and assert RIGHT_ARROW not in rendered_lines[2]); this keeps the intent via the
rendered_lines variable while satisfying the linter RUF001.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4d4f4501-dd43-4644-ac66-37f0622d2243
⛔ Files ignored due to path filters (1)
docs/en/reference/telemetry.mdis excluded by!docs/**
📒 Files selected for processing (9)
CHANGELOG.mdCLAUDE.mdsrc/pythinker_code/telemetry/__init__.pysrc/pythinker_code/telemetry/sink.pysrc/pythinker_code/ui/shell/prompt.pytests/telemetry/test_instrumentation.pytests/telemetry/test_telemetry.pytests/ui_and_conv/test_prompt_tips.pytests/ui_and_conv/test_slash_completer.py
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Address CodeRabbit review feedback on PR #129: - flush_sync(): replace the blanket `suppress(Exception)` around the pre-sink queue drain with an explicit try/except that logs at debug and only clears `_event_queue` after a successful emit, so a failed hand-off no longer silently drops startup-crash telemetry. - emit_events_to_otel(): log the OTel import failure (with event count) before dropping events instead of returning silently. - test_slash_completer: build the selection marker via chr(0x276F) instead of the literal glyph to satisfy the ambiguous-character lint. - Add a test asserting the queue is retained when the crash-safe emit raises.
elkaix
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Two related improvements bundled in this branch:
Telemetry — crash-safe error severity
emit_events_to_otel()centralizes event forwarding: maps event name → OTel severity (error/warning/info), enriches canonicalerror.*attributes for error-like events, validates schemas, and passes explicit severity tootel.emit_log.flush_sync()now detects buffered events when no sink is attached (e.g. a crash during startup beforeattach_sink()), backfillsdevice_id/session_id, and emits directly to OTel so the crash event still reaches SigNoz. The queue is only cleared after a successful emit; failures are logged at debug rather than swallowed.UI — slash command menu
[shell]/[command]scope tags, keeping onlyskill:/flow:kind tags.Checklist