Skip to content
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

Merged
merged 19 commits into from
Nov 15, 2024

Conversation

odeke-em
Copy link
Contributor

@odeke-em odeke-em commented Nov 9, 2024

An important feature for observability is to allow the injection of a custom tracer_provider instead of always using the global tracer_provider.

@odeke-em odeke-em requested review from a team as code owners November 9, 2024 19:22
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: spanner Issues related to the googleapis/python-spanner API. labels Nov 9, 2024
@odeke-em odeke-em force-pushed the tracer_provider-injection branch 5 times, most recently from eca7d36 to 549cfaa Compare November 11, 2024 06:05
@harshachinta harshachinta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 11, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 11, 2024
@odeke-em odeke-em force-pushed the tracer_provider-injection branch 3 times, most recently from 285c578 to 52c4b3b Compare November 11, 2024 08:49
@harshachinta harshachinta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 11, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 11, 2024
Copy link
Contributor

@harshachinta harshachinta left a 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.

google/cloud/spanner_v1/_opentelemetry_tracing.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/_opentelemetry_tracing.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/_opentelemetry_tracing.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/client.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/instance.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/instance.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/pool.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/pool.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/pool.py Outdated Show resolved Hide resolved
@odeke-em
Copy link
Contributor Author

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!

@odeke-em odeke-em force-pushed the tracer_provider-injection branch 3 times, most recently from 75a165a to 3d35c4d Compare November 12, 2024 06:09
@odeke-em
Copy link
Contributor Author

@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.

@odeke-em odeke-em force-pushed the tracer_provider-injection branch 3 times, most recently from 3c3ed5a to 529d7c8 Compare November 12, 2024 07:42
google/cloud/spanner_v1/client.py Show resolved Hide resolved
google/cloud/spanner_v1/database.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/snapshot.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/snapshot.py Show resolved Hide resolved
google/cloud/spanner_v1/transaction.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/_opentelemetry_tracing.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/_opentelemetry_tracing.py Outdated Show resolved Hide resolved
tests/unit/test__opentelemetry_tracing.py Outdated Show resolved Hide resolved
@odeke-em
Copy link
Contributor Author

Thank you for the review feedback @harshachinta! Kindly help me refresh the bots and take a look again, we should be go to go.

@harshachinta harshachinta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 14, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 14, 2024
@harshachinta harshachinta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 14, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 14, 2024
@harshachinta harshachinta changed the title all: implement custom tracer_provider injection feat(spanner): implement custom tracer_provider injection for opentelemetry traces Nov 15, 2024
@harshachinta harshachinta merged commit 6869ed6 into googleapis:main Nov 15, 2024
10 of 12 checks passed
@odeke-em odeke-em deleted the tracer_provider-injection branch November 15, 2024 04:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/python-spanner API. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants