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

fix(tracing): ensure nesting of Transaction.begin under commit + fix suggestions from feature review #1287

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

odeke-em
Copy link
Contributor

@odeke-em odeke-em commented Jan 9, 2025

This change ensures that:

  • If a transaction was not yet begin, that if .commit() is invoked the resulting span hierarchy has .begin nested under .commit
  • We use "CloudSpanner.Transaction.execute_sql" instead of "CloudSpanner.Transaction.execute_streaming_sql"
  • If we have a tracer_provider that produces non-recordings spans, that it won't crash due to lacking span._status

Fixes #1286

@odeke-em odeke-em requested review from a team as code owners January 9, 2025 10:50
@product-auto-label product-auto-label bot added size: l Pull request size is large. api: spanner Issues related to the googleapis/python-spanner API. labels Jan 9, 2025
@harshachinta harshachinta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 9, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 9, 2025
@odeke-em odeke-em force-pushed the trace-update-cases-from-review branch from e83b4af to 423e5bc Compare January 9, 2025 11:01
@harshachinta harshachinta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 9, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 9, 2025
tests/system/test_observability_options.py Outdated Show resolved Hide resolved
tests/unit/test_database.py Outdated Show resolved Hide resolved
tests/unit/test_transaction.py Outdated Show resolved Hide resolved
tests/unit/test_transaction.py Outdated Show resolved Hide resolved
tests/unit/test_database.py Outdated Show resolved Hide resolved
@harshachinta harshachinta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 10, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 10, 2025
@odeke-em odeke-em force-pushed the trace-update-cases-from-review branch from d32aab2 to 68cae9d Compare January 10, 2025 08:47
@odeke-em odeke-em requested a review from harshachinta January 10, 2025 08:49
@odeke-em odeke-em force-pushed the trace-update-cases-from-review branch from 68cae9d to 44b2862 Compare January 10, 2025 09:03
@harshachinta harshachinta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 10, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 10, 2025
…suggestions from feature review

This change ensures that:
* If a transaction was not yet begin, that if .commit() is invoked
the resulting span hierarchy has .begin nested under .commit
* We use "CloudSpanner.Transaction.execute_sql" instead of
  "CloudSpanner.Transaction.execute_streaming_sql"
* If we have a tracer_provider that produces non-recordings spans,
that it won't crash due to lacking `span._status`

Fixes googleapis#1286
@odeke-em odeke-em force-pushed the trace-update-cases-from-review branch 2 times, most recently from 8c27fc6 to 8ac5d36 Compare January 10, 2025 12:32
@odeke-em
Copy link
Contributor Author

@harshachinta kindly please help me run the bots on this. Thank you.

@sakthivelmanii sakthivelmanii added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 10, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 10, 2025
@odeke-em odeke-em force-pushed the trace-update-cases-from-review branch from 8ac5d36 to 56f97a2 Compare January 10, 2025 16:22
@odeke-em
Copy link
Contributor Author

@sakthivelmanii @harshachinta kindly help me run those bots once more.

@harshachinta harshachinta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 10, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 10, 2025
@odeke-em
Copy link
Contributor Author

Another re-run of the bots, kindly @harshachinta @sakthivelmanii @alkatrivedi

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
5 participants