Conversation
2401aa4 to
e497568
Compare
1. Merge Tracing & CaptureException middlewares 2. Rename `CaptureException` to `CaptureExceptions` to follow other Rails middlewares' naming convention, like `ActionDispatch::ShowExceptions`
e497568 to
ab8851b
Compare
|
@st0012 when Dependabot tried to bump us to Sentry 4.1.0 (from 4.0.1), we started getting constant loading errors: I wonder if there is a load order issue here that needs to be resolved with an explicit |
|
@agrobbin it's because your |
|
Ohhhh! The downside of Dependabot only upgrading 1 dependency at a time. Sorry for the noise! |
|
@agrobbin no worries, glad to help 😄 |
|
I wonder, does this indicate that the gemspec dependencies of |
|
that's a good point, let me see if I can patch a release for that 👍 |
|
I've added a PR for this #1159. I'll wait until next week for the |
|
@st0012 awesome! Actually, there might be another small bug. I haven't had a chance to dig into it too much yet, but even after updating Sorry for occupying your entire morning! |
Here's a full description on the issue #1147 (comment)
Here's a full description on the issue #1147 (comment)
|
No worries, thanks @st0012! |
|
@dentarg sorry that this change broke the semver spec, but this one was quite tough to call. let me explain it a bit: there's a bug (#1153) that requires this breaking change. but given that we've just released v4, bumping it to v5 right away without any major functional changes seems to be more confusing. and the other consideration is that if the users also use but I also know that the migration is not smooth and can be disappointing to some people, like @agrobbin's report above (sorry 😢). so I added #1159 #1160 with the hope to improve this and future upgrade's experience. and we'll be working harder to avoid similar situations from happening again. so my answer is: we'll try to follow it as closely as possible, and I'm sorry for breaking it this time. such issues should happen less and less after the new SDK is becoming more stable (which will be soon, I promise). |
|
@st0012 thanks for trying to explain but I have a hard time understanding a decision like the above if one has any intention to respect SemVer. Not wanting to release a new major version without new major features sounds to me like marketing. I can understand that as reason, but it doesn't squash with SemVer – I think you should be honest about that. I don't think it is confusing to release a new major version when there's breaking changes, people will read the changelog and understand what's happening. Encountering breaking changes when not updating the major version, that's confusing (unless you know the gem doesn't respect SemVer, then you are more careful). I understand (hope) that you wont reason the same when I'll connect this PR with the documentation updates getsentry/sentry-docs#2791 that shows what changed with 4.1.0. |
| Sentry.clone_hub_to_current_thread | ||
|
|
||
| Sentry.with_scope do |scope| | ||
| scope.clear_breadcrumbs |
There was a problem hiding this comment.
@matthieuprat it's a way to make sure the request scope doesn't contain any pre-existed data. of course, it can be improved. does this approach cause any issue in your app?
There was a problem hiding this comment.
My apologies, I should have provided more context with my question in the first place!
It doesn't cause any issues on our side. I was asking merely because we drew inspiration from that middleware to track errors in our custom job runner and we weren't sure whether this particular line was relevant to us. I also thought that maybe this PR actually made it unnecessary because the clone_hub_to_current_thread method was now being called for every request.
In fact, my understanding from your answer is that we do need it, otherwise, the breadcrumbs could include data from before the main hub was cloned for the first time.
Out of curiosity, why aren't we doing the same thing in the ActiveJob integration?
There was a problem hiding this comment.
@matthieuprat sorry for the delay. it's because I didn't see the same breadcrumb data slipped into AJ events. I didn't dig into the reason though so I may be wrong.
There was a problem hiding this comment.
No worries! OK, makes sense. (I haven't tested the AJ integration myself so can't tell whether this is an actual issue.)
CaptureExceptiontoCaptureExceptionsto follow other Rails middlewares' naming convention, likeActionDispatch::ShowExceptions