Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrap rails runner in executor #44999

Merged
merged 1 commit into from Jun 10, 2022
Merged

Wrap rails runner in executor #44999

merged 1 commit into from Jun 10, 2022

Conversation

casperisfine
Copy link
Contributor

The main reason is to automatically report uncaught exceptions since rails runner is often used for cron tasks and such.

cc @st0012

@st0012
Copy link
Contributor

st0012 commented May 5, 2022

So it's only available through the error reporter right? I think we still don't have document for it feature yet. Do you think we can add a Reporting Errors page under the Digging Deeper section of Rails guide?

@casperisfine
Copy link
Contributor Author

So it's only available through the error reporter right? I

Sorry, I don't think I understand your question.

Do you think we can add a Reporting Errors page under the Digging Deeper section of Rails guide?

Maybe?

@st0012
Copy link
Contributor

st0012 commented May 5, 2022

I thought it'd be a new API in the Railtie like I proposed here.

Capturing it with the executor will work as well, but it also means the SDKs need to opt-in the error reporter to get it (which I think will encourage more SDKs to opt-in).

However, because it's reported via the execution wrapper, we only get unhandled_error.active_support as the source.

I hope Rails can provide more accurate source whenever possible, which in this case seems to be possible?

Rails.error_reporter.record(source: "rails_runner.railties") do
  # runner execution
end

Wdyt?

@casperisfine
Copy link
Contributor Author

Wdyt?

I don't think it's necessary. source makes sense for handled errors you might want to ignore. Once an error isn't handled, I don't see a use case for ignoring them.

@casperisfine
Copy link
Contributor Author

Also, for the user reading the report, the backtrace should make the origin of the error obvious.

@st0012
Copy link
Contributor

st0012 commented May 6, 2022

Sorry I should've explained: I'm also thinking about using the source as a tag of the error events. In Sentry, tagging is one of the most commonly used filtering methods. The same concept also exists on some other platforms like Appsignal.

With the source tag, users can easily browse errors based on their sources (e.g. all Rails runner errors, all library errors), which will be a great addition imo. That's why I want to have accurate source info.

@byroot
Copy link
Member

byroot commented May 12, 2022

Ok, I think you convinced me again, so I added a distinct source: for each wrap. Let me know what you think.

@@ -60,7 +60,7 @@ class Engine < Rails::Engine # :nodoc:
initializer "action_cable.set_work_hooks" do |app|
ActiveSupport.on_load(:action_cable) do
ActionCable::Server::Worker.set_callback :work, :around, prepend: true do |_, inner|
app.executor.wrap do
app.executor.wrap(source: "unhandled_error.action_cable") do
Copy link
Contributor

@st0012 st0012 May 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To Sentry, the unhandled_error prefix is not necessary as we'd add another tag called handled to the event. So users can use handled + source tag for searching.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, in this case the unhandled_error is to signal that the application, not the framework, let something bubble up.

There might be a better name for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think if we set the naming rules like [app.][feature.]<component>. For example:

  • app.action_cable - user's action_cable logic
  • action_cable - action_cable internal (if possible)
  • redis_cache_store.active_support
  • app.runner.railties - user's runner code

I try to mimic the same pattern as the ActiveSupport instrumentation has. But at here the <function> is not always available. And we need an extra flag to indicate if it's the user's code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd likely go with application over app, it's longer, but Rails doesn't use that much. But yes I like the general idea.

@st0012
Copy link
Contributor

st0012 commented May 14, 2022

Thanks for making the change, I think it'll be super helpful for our users!
Do you know when this will likely go out? I assume the end of the year like last year? I'll schedule a time to write the document for this on Rails guide and Sentry's doc.

@st0012
Copy link
Contributor

st0012 commented May 29, 2022

@casperisfine After this is merged, I'll push a rails-main branch to the SDK based on our prev discussions and start finding people to give it a try. And if there will be another pre-release version that contains these changes it'd be very helpful.
I'll also start drafting relevant document for this feature.

The main reason is to automatically report uncaught exceptions
since `rails runner` is often used for cron tasks and such.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants