-
Notifications
You must be signed in to change notification settings - Fork 373
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
Fix concurrent ruby future propagation without active_trace
#3242
Fix concurrent ruby future propagation without active_trace
#3242
Conversation
@@ -135,6 +135,18 @@ | |||
end | |||
end | |||
end | |||
|
|||
context 'when propagates without an active trace' do | |||
it 'creates a root span' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix looks good but that sounds weird. I would expect:
- 'does nothing'
- 'does not raise'
- 'calls executor'
|
||
future.wait | ||
|
||
expect(inner_span).to be_root_span |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that it indirectly test that it did not crash (because if it crashed the test would blow up), but does that actually test that it didn't append a span to the trace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can address @lloeki concerns, but the fix itself looks good.
I am merging this to dish out a release as soon as possible. |
What does this PR do?
Concurrent ruby propagation breaks with
NoMethodError
when there is noactive_trace
to propagates.Fix with conditionals.