- 
                Notifications
    
You must be signed in to change notification settings  - Fork 568
 
Add external_propagation_context support #5051
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
Conversation
77c1d24    to
    5c675fd      
    Compare
  
    
          Codecov Report❌ Patch coverage is  
 Additional details and impacted files@@            Coverage Diff             @@
##           master    #5051      +/-   ##
==========================================
- Coverage   83.93%   83.91%   -0.03%     
==========================================
  Files         179      179              
  Lines       17887    17894       +7     
  Branches     3179     3177       -2     
==========================================
+ Hits        15013    15015       +2     
- Misses       1906     1908       +2     
- Partials      968      971       +3     
  | 
    
e902c81    to
    3763d47      
    Compare
  
    If we are on an external tracing system like otel, we allow registering a new source of `trace_id/span_id` that takes precedence over the scope's propagation_context. * Also reworked logs and metrics to use `get_trace_context` * Cleaned up handling of `get_trace_context` that is still messy but a bit more clearer now regarding which underlying `propagation_context` is used
3763d47    to
    203d4d1      
    Compare
  
    | 
               | 
          ||
| # Add "trace" context | ||
| if contexts.get("trace") is None: | ||
| if has_tracing_enabled(options) and self._span is not None: | 
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.
moved these into get_trace_context for consistency
| 
               | 
          ||
| return trace_context | ||
| propagation_context = self.get_active_propagation_context() | ||
| if propagation_context is None: | 
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.
propagation_context is actually never None, but this implementation is whack so we need this redundant check...
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.
for some future cleanup, if you see get_active_propagation_context that's also really shady. I generally dislike that so many instance methods on this scope call and fetch other scopes themselves, it's entirely spaghetti but somehow works in the common code paths.
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.
Looking good as far as I can tell. Thanks for centralizing the logic more, def an improvement to directly accessing the propagation context from random places 😢
If we are on an external tracing system like otel, we allow registering a new source of
trace_id/span_idthat takes precedence over the scope's propagation_context.get_trace_contextget_trace_contextthat is still messy but a bit more clearer now regarding which underlyingpropagation_contextis used