Allow custom tags to be passed when using the ActiveSupport::ErrorReporter

[BobbyMcWho]

Description

Allows for passing tags within the ActiveRecord::ErrorReporter context, to properly be sent to Sentry. I noticed that this file lacks spec coverage, should I add a spec file for it?

Q: context[:tags] or context[:sentry_tags] ?

[st0012]

Thanks for the PR 👍
I think we can use tags first as I can't think of any potential conflicts yet.

Can you also add a few cases for it? For example:

• It works when context[:tags] is a Hash and doesn't mutates context
• It doesn't break when context[:tags] is a non-Hash

[BobbyMcWho]

@st0012 Added the suggested changes, mind approving the workflow runs?

[codecov-commenter]

Codecov Report

Base: 98.41% // Head: 98.10% // Decreases project coverage by -0.31% ⚠️

Coverage data is based on head (26db53c) compared to base (f466ed3).
Patch coverage: 38.63% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1932      +/-   ##
==========================================
- Coverage   98.41%   98.10%   -0.32%     
==========================================
  Files         156      158       +2     
  Lines        9793     9847      +54     
==========================================
+ Hits         9638     9660      +22     
- Misses        155      187      +32     

Impacted Files	Coverage Δ
sentry-rails/lib/sentry/rails/error_subscriber.rb	38.46% <0.00%> (ø)
...y-rails/spec/sentry/rails/error_subscriber_spec.rb	41.46% <41.46%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[BobbyMcWho]

@st0012 does the inclusion of the spec break non rails-7 tests?

[st0012]

We also have integration tests for the error reporter here. We can still use the unit tests you added but you need to skip them for Rails 7.0-

[BobbyMcWho]

Alright @st0012, I believe cb032e9 should resolve the suite failures

[BobbyMcWho]

Is it just jruby that's failing now? Is it the new jruby release?

[st0012]

The JRuby failure should have been fixed on master.
The tests look good but I think we can still improve the implementation. Also, can you add a changelog entry for this PR? Thx

[st0012]

Looks good and thanks for the work 👍
We still need a changelog entry to merge this though 🙂

[BobbyMcWho]

Sorry about that @st0012, been majorly busy with things. If there's any way to get this into a 5.7.1 release that would be lovely, else we can pin to a ref I suppose.

[st0012]

@BobbyMcWho I think it's a feature so it needs to wait for the next minor/major release, which will probably happen around Christmas 🙂

[BobbyMcWho]

Okay, giving it a thought I can shut off the auto instrumented one and just add a custom subscriber to our app in the meantime, so that's fine!

[st0012]

I do want to merge it into master though so you can just use the GH source. But for some reason it doesn't allow me to merge even with all checks passing.
@sl0thentr0py Can you help me merge this? I'm not sure what's blocking it.

[sl0thentr0py]

hmm this is because danger can't update status on a forked PR

Warning: Running from a forked repo. Danger won’t be able to post comments and workflow status on the main repo, printing directly.

will see what to do