-
Notifications
You must be signed in to change notification settings - Fork 148
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
Allow setting component and action through the context #671
Comments
I think we should think about/discuss a bit more as this as a team. My main concern is that Looping in @subzero10 @rabidpraxis — what do you think? |
@joshuap agreed with your concerns regarding namespacing. Perhaps adding a |
I don’t have a strong opinion on this (and that’s probably why it took me a few days to reply 😅), but one way to approach this would be to first define what the ideal solution would look like without the constraints of the current implementation. Then, once we’ve settled on that, we can figure out how to bridge the gap between the current behavior and the ideal solution. If we look at component and action purely from a design perspective:
With that in mind, I think a safe approach could be:
This approach would:
|
@subzero10 completely agree. Perhaps using |
If https://docs.honeybadger.io/guides/errors/#context-data I just remembered that we also do something similar for Just trying to think through all the implications of this. I think I'm warming to the idea of keeping them more general top-level keys ( |
To clarify, with this you are saying that we could add the to the Another benefit with this solution would be that we can implement this change in the Honeybadger backend instead of having to add code in the client libraries to extract Finally, if I understand correctly, the only concern with this solution is that it could create confusion for customers already using |
Today I was working on a wordpress-plugin for Honeybadger and I just came across this in our PHP library. It seems that for the PHP library, we support setting
|
Before Rails 8 was released we used the following to explicitly set the component and action when calling
Rails.report
(to which, so far, only Honeybadger is subscribed):This worked perfectly, the component and action were being correctly rendered by the Honeybadger UI. This was especially useful for explicitly setting the component and action for our jobs (we cannot use the Sidekiq plugin for Honeybadger as we handle errors in a different way). Downside of course is that other error reporters, if we were going to use them, would probably choke on the payload not being an exception.
However, once Rails 8 was released this workaround stopped working. Reason being the addition of this line to
ActiveSupport::ErrorReporter
:https://github.com/rails/rails/blob/v8.0.1/activesupport/lib/active_support/error_reporter.rb#L212
This line breaks when the first argument passed in to
Rails.error.report
does not respond to thebacktrace
method. There are hacky solutions for this, but maybe it's better if it's possible to pass in the component and action through the context instead, and Honeybadger can retrieve it from the context? That would also be more compatible in the situation of other error reporters than Honeybadger subscribing.So something like:
What do you think?
The text was updated successfully, but these errors were encountered: