-
Notifications
You must be signed in to change notification settings - Fork 377
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
Extract context clearing logic for ActiveSupport events #897
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,6 +56,40 @@ def tracer | |
end | ||
end | ||
end | ||
|
||
# Extension to {Event} class that ensures the current {Context} | ||
# is always clean when the event is processed. | ||
# | ||
# This is a safeguard as Contexts are thread-bound. | ||
# If an integration re-uses threads, the context from a previous | ||
# execution could leak into the new execution. | ||
# | ||
# This module *cannot* be used for events can be nested, as | ||
# it drops all spans currently active in the {Context}. | ||
module RootEvent | ||
def subscription(*args) | ||
super.tap do |subscription| | ||
subscription.before_trace { ensure_clean_context! } | ||
end | ||
end | ||
|
||
private | ||
|
||
# Clears context if there are unfinished spans in it | ||
def ensure_clean_context! | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the idea of extracting this function out and standardizing this here. Does it make a sense to go a step further and extract this function to a module available to all integrations? Other integrations do the same, such as Rack and SuckerPunch, where they reset the context as well. If we're standardizing here, I don't think its a stretch to standardize everywhere, if plausible. |
||
unfinished_span = configuration[:tracer].call_context.current_span | ||
return unless unfinished_span | ||
|
||
Diagnostics::Health.metrics.error_unfinished_context( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we know how often this occurs roughly? It's not clear to me whether resetting the context is normal operating procedure, or an error as its been identified here. I think it'd be best to avoid emitting this as an error if this is normal; we should only submit things as errors if they are things that should be preventable or fixable. |
||
1, tags: [ | ||
"span_name:#{unfinished_span.name}", | ||
"event:#{self}" | ||
] | ||
) | ||
|
||
configuration[:tracer].provider.context = Context.new | ||
end | ||
end | ||
end | ||
end | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
RSpec.shared_context 'an unfinished trace' do | ||
let(:unfinished_span_name) { 'unfinished_span' } | ||
let!(:unfinished_span) { tracer.trace(unfinished_span_name) } | ||
|
||
before do | ||
expect(Datadog::Diagnostics::Health.metrics).to receive(:error_unfinished_context) | ||
.with(1, tags: [ | ||
"span_name:#{unfinished_span_name}", | ||
"event:#{event_name}" | ||
]) | ||
end | ||
end |
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.
Could we add unit tests for this module specifically? I'd like to see some assurance that if you use this module, it will prevent contexts from leaking.
I saw there's a test in Racecar, but I'd prefer something more explicit and directly related to this module, as its the component that owns this behavior.