Skip to content

Show debugger in crash reports - #24871

Merged
Jarred-Sumner merged 2 commits into
mainfrom
jarred/analytics-debugger
Nov 20, 2025
Merged

Jarred-Sumner merged 2 commits into
mainfrom
jarred/analytics-debugger

Conversation

@Jarred-Sumner

Copy link
Copy Markdown
Collaborator

What does this PR do?

Show debugger in crash reports

How did you verify your code works?

@robobun

robobun commented Nov 20, 2025 •

Copy link
Copy Markdown
Collaborator
Updated 9:25 PM PT - Nov 19th, 2025

❌ @Jarred-Sumner, your commit af5991b has 5 failures in Build #32085 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 24871

That installs a local version of the PR into your bun-24871 executable, so you can run:

bun-24871 --bun

@RiskyMH

RiskyMH commented Nov 20, 2025

Copy link
Copy Markdown
Contributor

does this show for vscode terminal one? as that may be slightly misleading, but at same time kinda true as it does do some extra debugger stuff

@coderabbitai

coderabbitai Bot commented Nov 20, 2025 •

Copy link
Copy Markdown
Contributor

Walkthrough

Added debugger usage tracking by introducing a new debugger counter field to the Features struct and incrementing it when debugger initialization occurs.

Changes

Cohort / File(s) Change Summary
Analytics tracking setup
src/analytics.zig
Added debugger field of type usize (initialized to 0) to Features struct
Debugger usage tracking
src/bun.js/Debugger.zig
Incremented bun.analytics.Features.debugger counter in waitForDebuggerIfNecessary function

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description includes both required sections but the 'How did you verify your code works?' section is empty, providing no verification details or testing information. Fill in the verification section with specific testing steps, reproduction instructions, or manual testing details to confirm the debugger tracking feature works correctly.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Show debugger in crash reports' is specific and clearly summarizes the primary change of adding debugger tracking to analytics.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 0054506 and af5991b.

📒 Files selected for processing (2)
  • src/analytics.zig (1 hunks)
  • src/bun.js/Debugger.zig (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-16T02:17:35.237Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23710
File: src/analytics.zig:15-21
Timestamp: 2025-10-16T02:17:35.237Z
Learning: In src/analytics.zig and similar files using bun.EnvVar boolean environment variables: the new EnvVar API for boolean flags (e.g., bun.EnvVar.do_not_track.get(), bun.EnvVar.ci.get()) is designed to parse and return boolean values from environment variables, not just check for their presence. This is an intentional design change from the previous presence-based checks using bun.getenvZ().

Applied to files:

  • src/analytics.zig
  • src/bun.js/Debugger.zig
📚 Learning: 2025-10-19T03:01:29.084Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:286-289
Timestamp: 2025-10-19T03:01:29.084Z
Learning: In src/bun.js/telemetry.zig, the guard preventing double configuration (lines 213-216) is intentional. The telemetry API uses a single-shot configuration model where configure() is called once during application startup. Users must call Bun.telemetry.configure(null) to reset before reconfiguring. This design ensures: (1) predictable state—callbacks don't change mid-request, avoiding race conditions; (2) zero overhead when disabled—no checking for callback changes on every request; (3) clear ownership—one adapter (e.g., bun-otel) owns the telemetry config. Merge-style reconfiguration would require atomic updates during active requests, adding overhead to the hot path.
<!-- [/add_learning]

Applied to files:

  • src/bun.js/Debugger.zig
🔇 Additional comments (2)
src/analytics.zig (1)

42-42: LGTM! Field declaration follows existing patterns.

The debugger counter is correctly declared and positioned alphabetically. It will be automatically included in packed_features_list and crash reports via the Formatter.

src/bun.js/Debugger.zig (1)

31-36: Verify the counting semantics for debugger usage.

The counter is incremented every time waitForDebuggerIfNecessary is called with a debugger present, which could result in multiple increments per debugging session if this function is invoked multiple times.

Consider: If the goal is to track debugger presence rather than usage frequency, incrementing in the create function (line 118) within the if (!has_created_debugger) block might be more appropriate, as it would count once per process.

However, if tracking the frequency of debugger access is intentional (e.g., to understand how often the wait path is hit), then the current placement is correct.

Can you clarify the intended counting behavior? Should this track:

  1. Presence: Debugger was enabled (increment once per process) → consider moving to create
  2. Frequency: How often the wait path is accessed (current implementation)

Also noting the comment from RiskyMH about VS Code terminal - the current implementation will indeed count VS Code's debugger actions, which seems aligned with tracking actual usage.


Comment @coderabbitai help to get the list of available commands and usage tips.

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