Extract context clearing logic for ActiveSupport events #897
Conversation
|
Now that #824 has been merged, I'm rebasing this on master instead. |
d7d5da1 to
cf0adf1
Compare
|
Rebased, ready for review again. |
| 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.
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.
| private | ||
|
|
||
| # Clears context if there are unfinished spans in it | ||
| def ensure_clean_context! |
There was a problem hiding this comment.
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.
| # | ||
| # This module *cannot* be used for events can be nested, as | ||
| # it drops all spans currently active in the {Context}. | ||
| module RootEvent |
There was a problem hiding this comment.
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.
When gems that utilize thread pools are instrumented it is very important to ensure that the thread-bound
Contextdoes not leak information between executions.We have existing logic to clear the
Contextfor a few integrations that instrument using ActiveSupport events.This PR extracts that logic to a central place, adds a health metric to count how many times the context is leaking and cleans up testing for integrations affected.