Deprecate capture_exception_frame_locals in favour of includeLocalVariables

[AbhiPrasad]

With getsentry/develop#813 merging in, we want to name the option includeLocalVariables in the Ruby SDK.

[st0012]

I actually prefer deprecating this feature until better APIs are provided by Ruby. Because the current implementation uses TracePoint and could largely affect Ruby YJIT's performance optimization (JITed code being thrown away after hitting the TracePoint), which is becoming more important after Ruby 3.2.

[AbhiPrasad]

Could we think about turning it off by default in the next major, but still giving users the option to turn it on?

[st0012]

Oh it's already off by default because there's always a concern on potential impact on performance. But now I'd actually discourage people from using it if they're on newer Rubies.

It also depends on how much we want to prioritise this feature in the longer-term. If it's something we want every language to support, we can invest more time on shaping the Ruby APIs and hope it'd be shipped in Ruby 3.3.

[AbhiPrasad]

From talking with Sentry users it's a pretty high value add for debugging, since it can add a lot of context into current app state. We regularly get feedback around people thinking it's high value (though this does tend to be for the Python ecosystem, maybe this is different in Ruby land). In general I think it's important for us to prioritize this.

Oh it's already off by default

That's my bad, thought this worked like PHP and Python. It's also off by default for Node as well at the moment!

If it's something we want every language to support, we can invest more time on shaping the Ruby APIs and hope it'd be shipped in Ruby 3.3.

It would be great if we could invest in the core Ruby ecosystem and get this in a better state. Is there any way we at Sentry can help with to make that smoother?

[st0012]

Yeah I agree that it's very valuable to have and that's why I implemented the current version. Glad to hear that it's been proven popular in other communities!

Is there any way we at Sentry can help with to make that smoother?

It has 2 stages:

1. Making the proposal accepted by the core team - I will try to move the discussion forward by pitching to more people and help shape the API names...etc.
   I think it's generally helpful to the entire community as I already came up with use cases for Rails and IRB. So IMO, it's likely be accepted. But if nobody wants to move forward on this, there won't be much we can do.
2. If it's accepted, we can help its development - In this case, we can either directly contribute to the gem, or be an early adopter to speed up the feedback loop, like what we did for Rails' Error Reporter feature (Sentry was the first adopter and I created that document page 🙂).

[HazAT]

@st0012 Let us know how we can support you (or generally) in this effort. Sentry needs this :)

[st0012]

Some updates about this feature:

• The proposal I mentioned above actually may not be suitable for production use, because keeping the frame data accessible will limit YJIT's optimisation options.
• On the other hand, the current implementation using TracePoint.new(:raise) actually doesn't cancel YJIT's optimisation. This is because unlike :line or :call's implementations, :raise event's doesn't trigger YJIT's invalidation logic.
  • Since it's not intentional to skip this event, there's no guarantee that it will always be like this. But it also doesn't look like it's going to change anytime soon.

So to summarise what we have now:

• I'm not sure if you're aware of it, but the current implementation ONLY captures the closest 1 frame's local variables, not the whole stack's. This is because we don't have a good way to access the frames on the stack. And based on the potential conflict with YJIT, there may never be one.
• Even though the current implementation doesn't cancel YJIT's optimisation, I think it's safer to do more production testing before turning it on by default.

I also want to say that cancelling YJIT's optimisation in the above case probably won't the end of the world. It's still largely depending on:

1. If it's a whole/partial cancellation.
   • My original concern about using TracePoint would've been a total cancellation, so that's why I was very worried.
2. How significant would the cancelled optimisation be.
3. Is there a good middle ground to let us trade the extra frame info with losing only few YJIT optimisation.

[st0012]

Since there's no immediate need to deprecate the feature, I opened #1993 for renaming.