Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jan 13, 2025
Merged

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

merged 3 commits into from
Jan 13, 2025

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
harshachinta
harshachinta reviewed Jan 10, 2025
View reviewed changes
@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
@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`

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

@sakthivelmanii sakthivelmanii added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 11, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 11, 2025
@harshachinta harshachinta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 13, 2025
@yoshi-kokoro yoshi-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:main Jan 13, 2025
10 of 12 checks passed
@release-please release-please bot mentioned this pull request Jan 13, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harshachinta harshachinta harshachinta approved these changes

Assignees

@harshachinta harshachinta

Labels
api: spanner Issues related to the googleapis/python-spanner API. size: l Pull 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