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

Allow setting component and action through the context #671

Open
gstokkink opened this issue Feb 20, 2025 · 8 comments · May be fixed by #672
Open

Allow setting component and action through the context #671

gstokkink opened this issue Feb 20, 2025 · 8 comments · May be fixed by #672

Comments

@gstokkink
Copy link

gstokkink commented Feb 20, 2025

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):

exception = ...
component = ...
action = ...
context = ...

payload = { exception:, component:, action: }

Rails.error.report(payload, context: ..., ...)

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 the backtrace 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:

exception = ...
component = ...
action = ...
context = ...

Rails.error.report(exception, context: context.merge(component:, action:), ...)

What do you think?

stympy added a commit that referenced this issue Feb 21, 2025
@stympy stympy linked a pull request Feb 21, 2025 that will close this issue
@stympy
Copy link
Member

stympy commented Feb 21, 2025

I think this is reasonable, and I threw together a possible implementation in #672.

@joshuap what do you think?

@joshuap
Copy link
Member

joshuap commented Feb 24, 2025

I think this is reasonable, and I threw together a possible implementation in #672.

@joshuap what do you think?

I think we should think about/discuss a bit more as this as a team. My main concern is that component and action are generic terms, and since context is user/domain-specific data, these terms could conflict with people's existing and future context data—in which case, it would change the grouping of those errors since we include the component in our fingerprint (as well as displaying them in the UI). We do currently have several "magic context keys" such as user_id and user_email, but I think this is a bit different since component and action are really internal Honeybadger properties.

Looping in @subzero10 @rabidpraxis — what do you think?

@gstokkink
Copy link
Author

@joshuap agreed with your concerns regarding namespacing. Perhaps adding a honeybadger namespace within the context hash is a good idea?

@subzero10
Copy link
Member

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:

  • They describe the context where the error happened, so it makes sense to treat them as contextual metadata rather than core error properties.
  • Since component and action aren’t inherent attributes of the exception itself (like message or backtrace), it feels natural to nest them within the context hash rather than passing them alongside the exception.

With that in mind, I think a safe approach could be:

  1. Prefix component and action inside the context hash (e.g., honeybadger_component, honeybadger_action) to avoid namespace collisions, as @gstokkink suggested.
  2. Extract those keys internally before sending the payload to Honeybadger, setting them on the top-level component and action fields as we do today.

This approach would:

  • Ensure compatibility with Rails 8 by sticking to the intended Rails.error.report API.
  • Avoid breaking users who might already be passing component or action in their own context data.
  • Maintain backwards compatibility without requiring users to update their existing error reporting code.

@gstokkink
Copy link
Author

@subzero10 completely agree. Perhaps using context: { honeybadger: { component:, action: }, ... } is more future-proof than context: { honeybadger_component:, honeybadger_action:, ... } when it comes to adding additional Honeybadger-specific contextual settings.

@joshuap
Copy link
Member

joshuap commented Feb 28, 2025

  • They describe the context where the error happened, so it makes sense to treat them as contextual metadata rather than core error properties.

  • Since component and action aren’t inherent attributes of the exception itself (like message or backtrace), it feels natural to nest them within the context hash rather than passing them alongside the exception.

If component and action really are context, I suppose we could make them official special keys, similar to user_id and user_email, and list them in our docs:

https://docs.honeybadger.io/guides/errors/#context-data

I just remembered that we also do something similar for tags in Ruby, so maybe we've already crossed this Rubicon?

https://docs.honeybadger.io/lib/ruby/getting-started/adding-context-to-errors/#special-context-values

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 (component and action) and adding it to the docs. 😅

@subzero10
Copy link
Member

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 (component and action) and adding it to the docs. 😅

To clarify, with this you are saying that we could add the to the context hash without namespacing them, right?
Considering that we already have support for special keys, this just means we are adding to that list, so yeah it makes sense to me.

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 component and action keys from context before sending them to Honeybadger (the second point of my suggested approach above).

Finally, if I understand correctly, the only concern with this solution is that it could create confusion for customers already using context.component and context.action with different values than the top-level component and action values. This seems unlikely to me, but I wonder if there's an easy way to verify this (i.e. query a sample of fairly recent errors)?

@subzero10
Copy link
Member

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 component and action through context, prefixed with honeybadger_, and extracting them just before sending the report to Honeybadger:

'request' => [
  // ....
  'context' => $this->context->except(['honeybadger_component', 'honeybadger_action']) ?: new stdClass,
  'component' => Arr::get($this->additionalParams, 'component', null) ?? Arr::get($this->context, 'honeybadger_component', null),
  'action' => Arr::get($this->additionalParams, 'action', null) ?? Arr::get($this->context, 'honeybadger_action', null),
],

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 a pull request may close this issue.

4 participants