ruby / debug

Debugging functionality for Ruby
BSD 2-Clause "Simplified" License
1.12k stars 126 forks source link

Feature Request: Push debugger activation to first breakpoint hit instead of `require "debug"` #797

Open st0012 opened 1 year ago

st0012 commented 1 year ago

Proposal

Why

Side-effects

In apps that have Bundler.require, like most Rails apps, having gem "debug" in Gemfile means the debug gem is always activated when the app is booted.

However, with the debugger's activation, many things will happen in the background:

To most apps, having them in the background doesn't make a huge difference. But to some, having the debugger activated causes tests to fail.

For example, I tested 5 mid-to-large Rails apps in Shopify, and 3 of them have test failures that only happen when debug is activated:

Therefore, we always have require: false after gem "debug".

Problem Solved?

However, having require: false means users need to manually type require "debug" before start debugging with console. But in the meantime, byebug can be required by default without the same side-effects. So byebug doesn't need that manual require.

This extra require step is quite inconvenient for people considering switching from byebug to debug.

Other Smaller Issues

Possible Change

diff --git a/lib/debug.rb b/lib/debug.rb
index 15ebccb..3489268 100644
--- a/lib/debug.rb
+++ b/lib/debug.rb
@@ -1,5 +1,3 @@
 # frozen_string_literal: true

 require_relative 'debug/session'
-return unless defined?(DEBUGGER__)
-DEBUGGER__::start no_sigint_hook: true, nonstop: true
diff --git a/lib/debug/session.rb b/lib/debug/session.rb
index a900f89..ffb2604 100644
--- a/lib/debug/session.rb
+++ b/lib/debug/session.rb
@@ -2486,7 +2486,9 @@ end

 module Kernel
   def debugger pre: nil, do: nil, up_level: 0
-    return if !defined?(::DEBUGGER__::SESSION) || !::DEBUGGER__::SESSION.active?
+    if !defined?(::DEBUGGER__::SESSION) || !::DEBUGGER__::SESSION.active?
+      DEBUGGER__::start no_sigint_hook: true, nonstop: true
+    end

     if pre || (do_expr = binding.local_variable_get(:do))
       cmds = ['binding.break', pre, do_expr]
ko1 commented 1 year ago

Let me think some more time about it.

ko1 commented 1 year ago

require "debug" won't activate the debugger. Debugger will only be activated when the first breakpoint is triggered.

Your proposal is based on that: user only specify the breakpoint with debugger method. I agree many people do that but I don't want to remove other possibilities.

To most apps, having them in the background doesn't make a huge difference. But to some, having the debugger activated causes tests to fail.

BTW requiring debug/prelude doesn't start session, but define debugger (and others) so you may want to use it (but it needs to change Gemfile a bit). Another idea is to make gem only requires debug/prelude.


I introduced RUBY_DEBUG_LAZY experimentally (so I don't write it on doc), it's require debug/prelude instead of calling start, so please try it and make it official on next version, or consider other possibilities.


The activation and forking messages are noisy and disabling them project-by-project is not trivial.

fixed on https://github.com/ruby/debug/pull/823

require "debug" locks the UI to console, so if users want to use the remote UI by requiring debug/open after the app is booted, require: false is also needed.

I'd supported it (override by requiring debug/open after requiring debug) but it was broken. fixed on https://github.com/ruby/debug/pull/825

st0012 commented 1 year ago

Your proposal is based on that: user only specify the breakpoint with debugger method.

If users want to set breakpoints with other ways, say with break command, shouldn't rdbg work the same? 🤔

I'm not sure why lazily loading debugger will reduce the possibilities to set breakpoints. Can you elaborate more?

In general, I want to solve these failures by fixing the debugger, not by lazy session starting.

Yes I also believe that's the way to go. But I'm not sure how much the effort it'd take and how it'd be prioritised.

I think lazy-activation to reduce friction is a good first step toward that goal though. And once we made activation lightweight, we can turn it back on and users wouldn't notice a thing.

I'll provide a way more lightweight version (disable collecting the source code, and so on).

I can test if disabling the TracePoint reduce test failures in our apps.

If we don't start the session, we couldn't support the following and I'm not sure it should be disabled by default.

I think the features you listed are useful, but also seem quite specialised. In my experience using and helping my colleagues use the debugger in apps and gems, we rarely need them. I'm not saying that we represent most users, but it may be a shared experience.

So how about we do it in another way:

The idea is, to make the debugger easily accessible to average users, we make it as lightweight as possible by default (lazy-activation is one way). And if some users want more powerful features, we also give them options to change the default behaviour. WDYT?

Another idea is to make gem only requires debug/prelude.

I think that's a good idea too 👍

fixed on https://github.com/ruby/debug/pull/823 fixed on https://github.com/ruby/debug/pull/825

Thank you so much for these fixes 🙏

ko1 commented 1 year ago

I'm not sure why lazily loading debugger will reduce the possibilities to set breakpoints. Can you elaborate more?

For example, rdbgrc can set breakpoints and editors (vi and so on) can set them.

The idea is, to make the debugger easily accessible to average users, we make it as lightweight as possible by default (lazy-activation is one way). And if some users want more powerful features, we also give them options to change the default behaviour. WDYT?

I believe reducing functionality is for high-level users who can understand what they are doing.

ko1 commented 1 year ago

for example: "if you want to use Regexp on Ruby, please use --regexp option" is not convenient.

st0012 commented 1 year ago

I believe reducing functionality is for high-level users who can understand what they are doing.

I think it really depends on what functionalities we are reducing and what are most users's usage of them.

For example, I haven't needed to use rdbgrc to set breakpoints. Because it means the breakpoint would be set to all Ruby projects I have, which varies a lot and it simply won't work. I'm not saying nobody uses this feature, but it's likely be a rare case. \

In the contrary, at Shopify we several applications, as well as gems like Tapioca, can benefit from having lazy-activation by default. (Also the app of my prev company, which's tests fail whenever debug is required).

I also want to add an update on the RUBY_DEBUG_LAZY experiment:

I experimented RUBY_DEBUG_LAZY=1 with the removal ofrequire: false on a big monolith we have. I applied it to

And the initial CI seemed to run fine, so we merged the PR.

But it turned out it randomly hanged some of the CI suites from time to time. And it took us a few days to pin down to my change. We don't exactly know what code was causing that, but it's loading the debugger that caused it. So we now added require: false back.

It's possible that we can add RUBY_DEBUG_LAZY=1 to the project's CI setup too. But that project has a dozen of CI suites. If I missed anyone of them, we'll have a flaky CI again.

What I want to say is, we're not requesting any advanced features of the debugger. We simply want to add gem "debug" to the Gemfile without require: false and let users explore it. But the current activation behaviour is causing a lot of issues just to do that.

The RUBY_DEBUG_LAZY experiment is helping, which we appreciate. But it'd still be a lot better if we can make it the default. And if the cost is some advanced and potentially rarely used features would require a flag to work, I think it's a reasonable tradeoff.

ko1 commented 1 year ago

But it turned out it randomly hanged some of the CI suites from time to time. And it took us a few days to pin down to my change. We don't exactly know what code was causing that, but it's loading the debugger that caused it. So we now added require: false back.

So RUBY_DEBUG_LAZY also have an issue you couldn't figure out, right?

st0012 commented 1 year ago

So RUBY_DEBUG_LAZY also have an issue you couldn't figure out, right?

Sorry I wasn't clear on this: the problem was caused by some CI tests not having RUBY_DEBUG_LAZY:

So my point is, needing to manually apply RUBY_DEBUG_LAZY in all the places that could use it could be a huge burden to users.

ko1 commented 1 year ago

BTW why do you use debug.gem on CI?

st0012 commented 1 year ago

In Rails applications, debug gems usually will be put under the development and test group in Gemfile. This is the Gemfile current Rails generates:

group :development, :test do
  # See https://guides.rubyonrails.org/debugging_rails_applications.html#debugging-with-the-debug-gem
  gem "debug", platforms: %i[ mri mingw x64_mingw ]
end

So when running tests, the debug gem could be automatically required and be used to debug tests. And because CI runs tests with test environment, the debug gem would be required there too.

ko1 commented 1 year ago

So you mean that

  1. most of programmers don't want to debug by debug.gem, especially on test group
  2. but it is required because Rails inserts it to Gemfile by default
  3. most of programmers don't want to use any debug.gem features until it calls debugger method

So the gem "debug.gem" is not assumed that "require it when debug with it" but introduce debugger command by many people, you say.

st0012 commented 1 year ago

Sorry for the delay.

So the gem "debug.gem" is not assumed that "require it when debug with it" but introduce debugger command by many people, you say.

Yes that's the feedback I got from many devs, also my personal expectation.

I understand that the goal is debug will become a completely built-in tool, which users won't need to add debug in their Gemfile at all. And when that happens, require "debug" could just mean "start debugging" and calling debugger would be redundant (like the most early design of debug did, which would start the console when required).

But at the moment and in near future, debug is still better to be treated as a gem because

Therefore, making its activation behaviour similar to other gems (e.g. pry, byebug) can make users adopting it quicker and benefit from its power.

ko1 commented 1 year ago

I understand that the goal is debug will become a completely built-in tool, which users won't need to add debug in their Gemfile at all.

At least I don't think such goal.

shioyama commented 10 months ago

Therefore, making its activation behaviour similar to other gems (e.g. pry, byebug) can make users adopting it quicker and benefit from its power.

I think if you want to make debug become the de-facto standard, this is pretty important.

eregon commented 10 months ago

Given that Bundler.require exists and is used e.g. by Rails, I think it's a fairly well-known expectation that all gems don't do heavy processing or e.g. add TracePoints while they are being loaded, as that could cause significant overhead or incompatibilities. So +1 for this.

I guess the main use case for require "debug" to start the debugger is for ruby -rdebug some_script.rb. Maybe that case can be detected and and so the session would be started eagerly only for that case?

When people add gem "debug" in their Gemfile, I would think none of them want that to immediately start the debugging session when they load their app.

eregon commented 10 months ago

ruby -rdebug some_script.rb

OTOH using rdbg ... for that use case seems good enough, so probably no need to treat ruby -rdebug any differently.

peterbrandel commented 5 months ago

Hey :wave:

Real world situation where we ran into an issue with how debug is being loaded: We were testing an app in a staging environment, introducing short lived background workers. Those background workers spawn a couple of worker processes per pod and execute the jobs asynchronously. To ensure everything was working as expected we ran a production load against the staging system, processing millions of events. Eventually the worker pods would run out of memory every time no matter how much memory was provided to the pod (went up to 16Gi and a single worker process per pod). After several days of scratching our heads we realized that the debug gem was included in the staging grouping in our Gemfile. Removing the debug gem from the staging environment resolved the issue.

Attaching a graph that shows how memory usage is shooting up as soon as the service gets deployed and is starting to run. The part where you see the everything drop in the end is when we removed debug from staging. The zig zag pattern is caused by deploys. While the service was running memory usage never dropped until we remove the gem.

Screenshot 2024-04-30 at 8 40 01 AM
st0012 commented 5 months ago

I also recently compared how the eager and lazy activation options would affect Rails application's test duration on CI, by using different gem declaration in Gemfile:

  1. gem "debug" (activated by default)
  2. gem "debug", require: "debug/prelude" (lazy activation, the proposed behaviour)

I ran full test suites of 3 different Rails apps, and saw between 9.1% to 11.17% extra test duration when debug is activated by default. For this reason, we've made require: "debug/prelude" a recommendation to all Rails apps at Shopify.

I think it's understandable that having a debugger running alongside the application would increase its memory consumption and cause extra overhead. But to me it means the debugger should by default either not be activated, or avoid the activities that would introduce such negative impacts.

And during a conversation with @ko1, he suggested that the overhead I observed may be caused by this line, which my later testings seemed to confirm as the 9~11% overhead largely disappeared. But I didn't do as thorough testing due to short of time, so we may need to verify that with more testings.

Notes