appsignal / appsignal-ruby

🟥 AppSignal for Ruby gem
https://www.appsignal.com/ruby
MIT License
180 stars 115 forks source link

Hybrid adding of errors #176

Closed tombruijn closed 2 months ago

tombruijn commented 8 years ago

Import from: https://github.com/appsignal/appsignal-ruby-private/issues/306

@thijsc

Customers are often confused on wether to use Appsignal.add_error or Appsignal.send_error.

For example: https://app.intercom.io/a/apps/yzor8gyw/inbox/thijs@appsignal.com/conversations/5610008461

We could add a third method Appsignal.track_error (or something like that) that:

  • Checks wether there is non-nil current transaction
  • Adds the error to the transaction if it exists
  • Sends the error if it does not exist

We can then deprecate the two current methods, or decide to keep everything around. Good idea?

@matsimitsu

hmm thing is you can (not that anyone does, but you can) add multiple errors for the same request. E.g. rescue an error, send it (with send_exception and then let the request continue. If another issue occurs then it will be added to the context.

What do we want to do with this one in the end?

We can always add a config option for the option to send errors directly or not

Appsignal.track_error(error, "push|now|immediatly" => true)

Idea:

matsimitsu commented 7 years ago

Another thing to keep in mind when we refactor this is that it would be nice to be able to set an error without having to create one first.

now:

class ImportError < StandardError; end
error = ImportError.new("Could not import #{feed.name}")
Appsignal.set_exception(error)

maybe later?

Appsignal.set_exception({
  :error_class => "ImportError",
  :error_message => "Could not import #{feed.name}",
})

This way you don't have to create error classes and instantiate objects.

ryansch commented 7 years ago

It's my understanding that you have to call set_backtrace on the error in the first example before you hand it off to set_exception as well.

tombruijn commented 7 years ago

Would be nice if we can combine this with Exception causes https://github.com/appsignal/appsignal-ruby/issues/121

It would also be nice to be able to track multiple errors on one transaction.

See also monitor_single_transaction, monitor_transaction, listen_for_error. Review the use-cases of this.

tombruijn commented 5 years ago

Allow users to give a block that returns the transactions.

Appsignal.send_error(error) do |transaction|
  transaction.set_namespace "admin"
end
Nowaker commented 3 years ago

I don't like the idea of set_error. It limits us to one error per transaction. I should be able to append an error to the list of errors Appsignal will send after the transaction finishes.

tombruijn commented 3 years ago

Hi @Nowaker, we will support multiple errors is a future version of AppSignal for Ruby. We're already working on this in the Elixir and Node.js integrations. The data model for the Ruby gem is currently set up in such a way that we only support one error per transaction. In the meantime I suggest using send_error to send individual errors.

tombruijn commented 2 months ago

Most recent internal discussion about this: https://appsignal.slack.com/archives/C7XHXQ7N0/p1719901148620369

Nowaker commented 2 months ago

@tombruijn What about this? https://github.com/appsignal/appsignal-ruby/issues/176#issuecomment-702372803

tombruijn commented 2 months ago

@Nowaker I didn't address supporting multiple errors in one transaction in this issue as it wasn't part of the original problem. I agree that it's annoying we don't support multiple errors in one transaction. Supporting that would mean many changes to an old data protocol the Ruby gem uses. Instead I'm floating the idea in the team of duplicating transactions for multiple errors. We'll discuss this implementation during our next team meeting. I've created a new tracking issue for this in #1152.

tombruijn commented 2 months ago

This was released in Ruby gem 3.10!