Disable tracing if events are not allowed to be sent#1421
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1421 +/- ##
==========================================
+ Coverage 98.21% 98.83% +0.61%
==========================================
Files 213 118 -95
Lines 9920 6173 -3747
==========================================
- Hits 9743 6101 -3642
+ Misses 177 72 -105
Continue to review full report at Codecov.
|
rhcarvalho
left a comment
There was a problem hiding this comment.
I think I generally understood the problem this is trying to solve and the current solution addresses the problem. 👍
However there's one violation of the sampling rules, and we need to sync on how the config of the SDK works with regards to tracing.
| it "sets @sampled to false and return" do | ||
| transaction = described_class.new(sampled: true, hub: Sentry.get_current_hub) | ||
| transaction.set_initial_sample_decision(sampling_context: {}) | ||
| expect(transaction.sampled).to eq(false) |
There was a problem hiding this comment.
Thought about this for a while. At first I thought neither true nor false fit here, because both of those mean a sampling decision was actually made, in which case nil would be more appropriate.
However, the sampling rule is that explicit sample decision always has precedence:
- If a sampling decision is passed to startTransaction (startTransaction({name: "my transaction", sampled: true})), that decision will be used, regardlesss of anything else
So in this particular example, since the sampling decision was passed to .new, it should not have changed.
In any case, tracing is effectively disabled, startTransaction should not instantiate transactions.
There was a problem hiding this comment.
I've addressed the decision making issue in #1423
There was a problem hiding this comment.
Thanks @st0012. I've chatted with the team and looked at code in other SDKs, I retract my comment for now.
Python and JS do exactly as Ruby does now, despite not adhering 100% to the spec as written.
So instead of going back and forth with this, let's just leave it as it is for now, and revisit it later as we rethink tracing as a whole.
There was a problem hiding this comment.
Yeah, I implemented this behavior based on what I saw in sentry-python. I thought the spec you posted was to correct that.
Users can enable/disable the SDK's event sending with `enabled_environments` config option. And when the current environment doesn't match the `enabled_environments`, the SDK will not send any events. But currently, the SDK would still start transactions under such condition. Even though those transactions would not be sent later (stopped by `Client#send_event`), it'd still create sampling logs, which are noisy and confusing to users. So this commit fixes the issue by making tracing activation aware of the event sending state.
01e68ed to
15458a3
Compare
Users can enable/disable the SDK's event sending with
enabled_environmentsconfig option. And when the current environment doesn't match theenabled_environments, the SDK will not send any events. But currently, the SDK would still start transactions under such condition.Even though those transactions would not be sent later (stopped by
Client#send_event), it'd still create sampling logs, which are noisy and confusing to users.So this commit fixes the issue by making tracing activation aware of the event sending state.