-
Notifications
You must be signed in to change notification settings - Fork 88
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
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.