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

Use report_error in Rails error subscriber #1240

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

tombruijn
Copy link
Member

The Rails error subscriber had already some kind of duplicate transaction behavior, when we didn't support multiple errors.

Remove the transaction duplication logic and use
Appsignal.report_error instead, to report the error. This will duplicate the transaction metadata more accurately when the transaction is closed, rather than whenever the error is reported. It will also duplicate all the sample data, not just tags.

If a "context" is given to Rails.error.handle, it will overwrite the data on the transaction with the given data. This is mostly useful for errors outside of an instrumented context, when a new transaction is created by Appsignal.report_error, and shouldn't be necessary for most other cases.

[skip changeset]

@tombruijn tombruijn self-assigned this Aug 9, 2024
@tombruijn tombruijn mentioned this pull request Aug 9, 2024
82 tasks
The Rails error subscriber had already some kind of duplicate
transaction behavior, when we didn't support multiple errors.

Remove the transaction duplication logic and use
`Appsignal.report_error` instead, to report the error. This will
duplicate the transaction metadata more accurately when the transaction
is closed, rather than whenever the error is reported. It will also
duplicate all the sample data, not just tags.

If a "context" is given to `Rails.error.handle`, it will overwrite the
data on the transaction with the given data. This is mostly useful for
errors outside of an instrumented context, when a new transaction is
created by `Appsignal.report_error`, and shouldn't be necessary for most
other cases.

[skip changeset]
@tombruijn tombruijn force-pushed the rails-error-subscribe-report-error branch from 660d560 to 6e83ae6 Compare August 9, 2024 15:08
@tombruijn tombruijn marked this pull request as ready for review August 9, 2024 15:27
Copy link
Contributor

@unflxw unflxw left a comment

Choose a reason for hiding this comment

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

Screenshot 2024-08-12 at 14 24 18

💯

@tombruijn tombruijn merged commit d4655df into develop Aug 12, 2024
113 checks passed
@tombruijn tombruijn deleted the rails-error-subscribe-report-error branch August 14, 2024 13:41
ryansch added a commit to detaso/appsignal-ruby that referenced this pull request Aug 14, 2024
* upstream/develop: (33 commits)
  Bump agent to version 0.35.21 (appsignal#1244)
  Remove heartbeat aliases and warnings
  Publish package v3.13.0
  Amend `@since` version number
  Add Rails 7.2 to CI build (appsignal#1242)
  Bump Ruby versions in the CI (appsignal#1241)
  Use new property names
  Remove initial config from Config class
  Add new build_config spec helper
  Update Capistrano integrations config
  Validate config right before start
  Do not auto validate config on initialize
  Remove app env key logic from Config initializer
  Remove custom config file config parameter
  Remove Appsignal.config= writer
  Record load and other errors on Rails app load
  Mark app as Rails app in diagnose report
  Use report_error in Rails error subscriber (appsignal#1240)
  Do not report HTTP gem errors (appsignal#1239)
  Load Rails config in diagnose CLI
  ...
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.

3 participants