Allow custom tags to be passed when using the ActiveSupport::ErrorReporter#1932
Conversation
st0012
left a comment
There was a problem hiding this comment.
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 mutatescontext - It doesn't break when
context[:tags]is a non-Hash
|
@st0012 Added the suggested changes, mind approving the workflow runs? |
Codecov ReportBase: 98.41% // Head: 98.10% // Decreases project coverage by
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
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. |
9671040 to
63bcfbe
Compare
|
@st0012 does the inclusion of the spec break non rails-7 tests? |
|
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- |
|
Is it just jruby that's failing now? Is it the new jruby release? |
st0012
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
Looks good and thanks for the work 👍
We still need a changelog entry to merge this though 🙂
…s-using-active_support-error_reporter
73c197c to
30f4273
Compare
|
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. |
|
@BobbyMcWho I think it's a feature so it needs to wait for the next minor/major release, which will probably happen around Christmas 🙂 |
|
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! |
|
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. |
|
hmm this is because danger can't update status on a forked PR
will see what to do |
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]orcontext[:sentry_tags]?