- 
                Notifications
    
You must be signed in to change notification settings  - Fork 101
 
feat(spanner): implement custom tracer_provider injection for opentelemetry traces #1229
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
feat(spanner): implement custom tracer_provider injection for opentelemetry traces #1229
Conversation
eca7d36    to
    549cfaa      
    Compare
  
    285c578    to
    52c4b3b      
    Compare
  
    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.
The current implementation creates a tight coupling between observability_options and session, even though they are unrelated. While this approach achieves the goal and simplifies the code (since it assumes a session is available in every method), and assigning observability_options to the session can make retrieval easier, it may lead to confusion for future developers.
I propose following the approach used in other languages. We should set observability_options at the transaction level whenever a transaction is created in database.py. Specifically, observability_options would be accessible in database.py, where customers call methods like snapshot(), batch(), and run_in_transaction(). These methods have base classes, such as _SnapshotBase, _BatchBase, etc. The observability_options would be set at this class level, and methods within these classes would access the options directly from the class whenever trace_call is invoked.
67adff0    to
    fef29ea      
    Compare
  
    | 
           Awesome and thank you very much @harshachinta for the insightful review! Roger that, I have made the updates to just infer observability_options from Instance itself. Please take another look. Thank you!  | 
    
75a165a    to
    3d35c4d      
    Compare
  
    | 
           @harshachinta so a compromise/meet-in-the-middle would being able to just infer it directly from the Client instead of plumbing all within Transaction creator. I mention this because having made the proposed change within database for where transactions are createdd, it is firstly much more code but also requires much more plumbing inside for example database.run_in_transaction for which then has to also modify session.run_in_transaction instead of simply just inside trace_call retrieving it from session._database.observability_options.  | 
    
3c3ed5a    to
    529d7c8      
    Compare
  
    8d3ca28    to
    8c5b50b      
    Compare
  
    | 
           Thank you for the review feedback @harshachinta! Kindly help me refresh the bots and take a look again, we should be go to go.  | 
    
8c5b50b    to
    165fdfd      
    Compare
  
    165fdfd    to
    196e9a0      
    Compare
  
    
An important feature for observability is to allow the injection of a custom tracer_provider instead of always using the global tracer_provider.