Skip to content
This repository was archived by the owner on Jun 8, 2026. It is now read-only.

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

Merged
harshachinta merged 3 commits into
googleapis:mainfrom
odeke-em:trace-update-cases-from-review
Jan 13, 2025
Merged

fix(tracing): ensure nesting of Transaction.begin under commit + fix suggestions from feature review#1287
harshachinta merged 3 commits into
googleapis:mainfrom
odeke-em:trace-update-cases-from-review

Conversation

@odeke-em

@odeke-emodeke-em commented Jan 9, 2025

Copy link
Copy Markdown
Contributor

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 teamJanuary 9, 2025 10:50
@product-auto-labelproduct-auto-labelBot added size: l Pull request size is large. api: spanner Issues related to the googleapis/python-spanner API. labels Jan 9, 2025
@harshachintaharshachinta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 9, 2025
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 9, 2025
@odeke-em
odeke-emforce-pushed the trace-update-cases-from-review branch from e83b4af to 423e5bcCompareJanuary 9, 2025 11:01
@harshachintaharshachinta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 9, 2025
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 9, 2025
harshachinta
harshachinta reviewed Jan 10, 2025
View reviewed changes
Comment threadtests/system/test_observability_options.py Outdated
Comment threadtests/unit/test_database.py Outdated
Comment threadtests/unit/test_transaction.py Outdated
Comment threadtests/unit/test_transaction.py Outdated
Comment threadtests/unit/test_database.py Outdated
@harshachintaharshachinta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 10, 2025
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 10, 2025
@odeke-em
odeke-emforce-pushed the trace-update-cases-from-review branch from d32aab2 to 68cae9dCompareJanuary 10, 2025 08:47
@odeke-em
odeke-emforce-pushed the trace-update-cases-from-review branch from 68cae9d to 44b2862CompareJanuary 10, 2025 09:03
@harshachintaharshachinta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 10, 2025
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 10, 2025
@odeke-em
fix(tracing): ensure nesting of Transaction.begin under commit + fix …
6fba449
…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`
Fixesgoogleapis#1286
@odeke-em
odeke-emforce-pushed the trace-update-cases-from-review branch 2 times, most recently from 8c27fc6 to 8ac5d36CompareJanuary 10, 2025 12:32
@odeke-em

odeke-em commented Jan 10, 2025

Copy link
Copy Markdown
ContributorAuthor

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

@sakthivelmaniisakthivelmanii added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 10, 2025
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 10, 2025
@odeke-em
odeke-emforce-pushed the trace-update-cases-from-review branch from 8ac5d36 to 56f97a2CompareJanuary 10, 2025 16:22
@odeke-em

odeke-em commented Jan 10, 2025

Copy link
Copy Markdown
ContributorAuthor

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

@harshachintaharshachinta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 10, 2025
@yoshi-kokoroyoshi-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 commented Jan 11, 2025

Copy link
Copy Markdown
ContributorAuthor

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

@sakthivelmaniisakthivelmanii added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 11, 2025
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 11, 2025
@harshachintaharshachinta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 13, 2025
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 13, 2025
@harshachinta
harshachinta merged commit d9ee75a into googleapis:mainJan 13, 2025
@release-pleaserelease-pleaseBot mentioned this pull request Jan 13, 2025
Merged
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@harshachintaharshachintaharshachinta approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@harshachintaharshachinta

Labels

api: spannerIssues related to the googleapis/python-spanner API.size: lPull request size is large.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

tracing: update CloudSpanner.Transaction.execute_streaming_sql to execute_sql; start Transaction.commit span up top to ensure

5 participants

@odeke-em@harshachinta@olavloite@yoshi-kokoro@sakthivelmanii