Skip to content

Conversation

odeke-em
Copy link
Contributor

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

This change adds more descriptive and value adding spans to replace the generic CloudSpanner.ReadWriteTransaction. With this change, we add new spans:

  • CloudSpanner.Database.run_in_transaction
  • CloudSpanner.execute_pdml
  • CloudSpanner.execute_sql
  • CloudSpanner.execute_update

Exhibit

Given this code

def tx_dql_select(tx):
    try:
        tx.execute_update("DELETE FROM Singers WHERE 1=1")
    except:
        pass

    tx.execute_update(
        'INSERT INTO Singers(SingerId, FirstName, LastName) VALUES(1000, "Bryan", "Adams"), (2000, "Slash", "GunsNRoses")'
    )
    tx.update(
        "Singers",
        columns=["SingerId", "FirstName"],
        values=[["1", "Bryan"], ["2", "Slash"]],
    )
    rows = tx.execute_sql("SELECT * FROM Singers")
    for row in rows:
        print(row)

we now have this exhibit

Without gRPC instrumentation

Screenshot 2024-11-18 at 2 08 46 AM

With gRPC instrumentation

Screenshot 2024-11-18 at 2 12 26 AM

@odeke-em odeke-em requested review from a team as code owners November 18, 2024 10:09
@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 18, 2024
@odeke-em odeke-em force-pushed the trace-update-more-descriptive-Database.run_in_transaction-spans branch from bc1befe to 502d0e5 Compare November 18, 2024 22:55
@odeke-em
Copy link
Contributor Author

Kindly help me take a look @harshachinta and help me run the bots afresh!

Comment on lines 699 to 707
if span:
span.add_event("Starting BeginTransaction")
txn = api.begin_transaction(
session=session.name, options=txn_options, metadata=metadata
)
if span:
span.add_event(
"Completed BeginTransaction", {"transaction.id": txn.id}
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@harshachinta please take a look.

@odeke-em
Copy link
Contributor Author

@harshachinta kindly help me run the bots one more time, I sent up a fix!

@odeke-em odeke-em force-pushed the trace-update-more-descriptive-Database.run_in_transaction-spans branch 2 times, most recently from 8d613ee to f767159 Compare November 20, 2024 12:06
@sakthivelmanii sakthivelmanii added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 20, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 20, 2024
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Nov 22, 2024
Comment on lines 738 to 739
if isinstance(observability_options, dict):
observability_options["db_name"] = self.name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can do a much simpler thing instead of adding db_name manually at every place.
We have configured a @Property for observability_options in database.py

def observability_options(self):

Can we add a new field, db_name, to observability_options if it already exists? If the customer does not provide any observability_options, we can create a new dictionary with this field. With this we need not do it at multiple places, WDYT?

if isinstance(observability_options, dict):
observability_options["db_name"] = self.name
with trace_call(
"CloudSpanner.execute_partitioned_pdml",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem i see here is with every retry there will be a new span parent CloudSpanner.execute_partitioned_pdml which does not indicate that a retry is happening.
Instead we should capture the retries with in only one parent span CloudSpanner.execute_partitioned_pdml.

@odeke-em odeke-em force-pushed the trace-update-more-descriptive-Database.run_in_transaction-spans branch 3 times, most recently from beb8bb9 to d297b66 Compare November 28, 2024 07:41
@harshachinta harshachinta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 29, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 29, 2024
@odeke-em odeke-em force-pushed the trace-update-more-descriptive-Database.run_in_transaction-spans branch from be40273 to 47ffdfc Compare December 2, 2024 05:24
@harshachinta harshachinta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 2, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 2, 2024
@odeke-em odeke-em force-pushed the trace-update-more-descriptive-Database.run_in_transaction-spans branch from cd4a8b1 to 4829a9a Compare December 2, 2024 12:57
@product-auto-label product-auto-label bot removed the size: l Pull request size is large. label Dec 3, 2024
@product-auto-label product-auto-label bot added the size: xl Pull request size is extra large. label Dec 3, 2024
@odeke-em odeke-em force-pushed the trace-update-more-descriptive-Database.run_in_transaction-spans branch from 8d6dad0 to eecca2e Compare December 3, 2024 13:35
@harshachinta harshachinta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 4, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 4, 2024
@odeke-em odeke-em force-pushed the trace-update-more-descriptive-Database.run_in_transaction-spans branch 3 times, most recently from 204067a to 847d89b Compare December 6, 2024 11:30
odeke-em added a commit to odeke-em/python-spanner that referenced this pull request Dec 6, 2024
This change carves out parts of PR googleapis#1241 in smaller pieces to
ease with smaller reviews.
This change adds more span events, updates important spans
to make them more distinct like changing:

"CloudSpanner.ReadWriteTransaction" to more direct and more
pointed spans like:
* CloudSpanner.Transaction.execute_streaming_sql

Also added important spans:
* CloudSpanner.Database.run_in_transaction
* CloudSpanner.Session.run_in_transaction
odeke-em added a commit to odeke-em/python-spanner that referenced this pull request Dec 6, 2024
This change carves out parts of PR googleapis#1241 in smaller pieces to
ease with smaller reviews.
This change adds more span events, updates important spans
to make them more distinct like changing:

"CloudSpanner.ReadWriteTransaction" to more direct and more
pointed spans like:
* CloudSpanner.Transaction.execute_streaming_sql

Also added important spans:
* CloudSpanner.Database.run_in_transaction
* CloudSpanner.Session.run_in_transaction
odeke-em added a commit to odeke-em/python-spanner that referenced this pull request Dec 6, 2024
This change carves out parts of PR googleapis#1241 in smaller pieces to
ease with smaller reviews.
This change adds more span events, updates important spans
to make them more distinct like changing:

"CloudSpanner.ReadWriteTransaction" to more direct and more
pointed spans like:
* CloudSpanner.Transaction.execute_streaming_sql

Also added important spans:
* CloudSpanner.Database.run_in_transaction
* CloudSpanner.Session.run_in_transaction
odeke-em added a commit to odeke-em/python-spanner that referenced this pull request Dec 7, 2024
This change carves out parts of PR googleapis#1241 in smaller pieces to
ease with smaller reviews.
This change adds more span events, updates important spans
to make them more distinct like changing:

"CloudSpanner.ReadWriteTransaction" to more direct and more
pointed spans like:
* CloudSpanner.Transaction.execute_streaming_sql

Also added important spans:
* CloudSpanner.Database.run_in_transaction
* CloudSpanner.Session.run_in_transaction
odeke-em added a commit to odeke-em/python-spanner that referenced this pull request Dec 7, 2024
This change carves out parts of PR googleapis#1241 in smaller pieces to
ease with smaller reviews.
This change adds more span events, updates important spans
to make them more distinct like changing:

"CloudSpanner.ReadWriteTransaction" to more direct and more
pointed spans like:
* CloudSpanner.Transaction.execute_streaming_sql

Also added important spans:
* CloudSpanner.Database.run_in_transaction
* CloudSpanner.Session.run_in_transaction
odeke-em added a commit to odeke-em/python-spanner that referenced this pull request Dec 16, 2024
This change carves out parts of PR googleapis#1241 in smaller pieces to
ease with smaller reviews.
This change adds more span events, updates important spans
to make them more distinct like changing:

"CloudSpanner.ReadWriteTransaction" to more direct and more
pointed spans like:
* CloudSpanner.Transaction.execute_streaming_sql

Also added important spans:
* CloudSpanner.Database.run_in_transaction
* CloudSpanner.Session.run_in_transaction
harshachinta pushed a commit that referenced this pull request Dec 17, 2024
* observability: add updated span events + traace more methods

This change carves out parts of PR #1241 in smaller pieces to
ease with smaller reviews.
This change adds more span events, updates important spans
to make them more distinct like changing:

"CloudSpanner.ReadWriteTransaction" to more direct and more
pointed spans like:
* CloudSpanner.Transaction.execute_streaming_sql

Also added important spans:
* CloudSpanner.Database.run_in_transaction
* CloudSpanner.Session.run_in_transaction

* all: update review comments + show type for BeginTransaction + remove prints

* Remove requested span event "Using Transaction"

* Move attempts into try block

* Transform Session.run_in_transaction retry exceptions into events

* More comprehensive test for events and attributes for pool.get

* Add test guards against Python3.7 for which OpenTelemetry is unavailable + address test feedback

* Remove span event per mutation in favour of future TODO

Referencing issue #1269, this update removes adding
a span event per mutation, in favour of a future TODO.

* Sort system-test.test_transaction_abort_then_retry_spans spans by create time

* Delint tests
This change adds more descriptive and value adding spans to
replace the generic CloudSpanner.ReadWriteTransaction.
With this change, we add new spans:
* CloudSpanner.Database.run_in_transaction
* CloudSpanner.execute_pdml
* CloudSpanner.execute_sql
* CloudSpanner.execute_update
@odeke-em odeke-em force-pushed the trace-update-more-descriptive-Database.run_in_transaction-spans branch from 847d89b to f28ed47 Compare December 17, 2024 10:38
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: xl Pull request size is extra large. labels Dec 17, 2024
odeke-em added a commit to odeke-em/python-spanner that referenced this pull request Dec 17, 2024
This change adds spans for Partitioned DML and making
updates for Batch.

Carved out from PR googleapis#1241.
odeke-em added a commit to odeke-em/python-spanner that referenced this pull request Dec 27, 2024
This change adds spans for Partitioned DML and making
updates for Batch.

Carved out from PR googleapis#1241.
aakashanandg pushed a commit to aakashanandg/python-spanner that referenced this pull request Jan 2, 2025
* observability: add updated span events + traace more methods

This change carves out parts of PR googleapis#1241 in smaller pieces to
ease with smaller reviews.
This change adds more span events, updates important spans
to make them more distinct like changing:

"CloudSpanner.ReadWriteTransaction" to more direct and more
pointed spans like:
* CloudSpanner.Transaction.execute_streaming_sql

Also added important spans:
* CloudSpanner.Database.run_in_transaction
* CloudSpanner.Session.run_in_transaction

* all: update review comments + show type for BeginTransaction + remove prints

* Remove requested span event "Using Transaction"

* Move attempts into try block

* Transform Session.run_in_transaction retry exceptions into events

* More comprehensive test for events and attributes for pool.get

* Add test guards against Python3.7 for which OpenTelemetry is unavailable + address test feedback

* Remove span event per mutation in favour of future TODO

Referencing issue googleapis#1269, this update removes adding
a span event per mutation, in favour of a future TODO.

* Sort system-test.test_transaction_abort_then_retry_spans spans by create time

* Delint tests
odeke-em added a commit to odeke-em/python-spanner that referenced this pull request Jan 9, 2025
This change adds spans for Partitioned DML and making
updates for Batch.

Carved out from PR googleapis#1241.
odeke-em added a commit to odeke-em/python-spanner that referenced this pull request Jan 10, 2025
This change adds spans for Partitioned DML and making
updates for Batch.

Carved out from PR googleapis#1241.
harshachinta pushed a commit that referenced this pull request Jan 10, 2025
* observability: PDML + some batch write spans

This change adds spans for Partitioned DML and making
updates for Batch.

Carved out from PR #1241.

* Add more system tests

* Account for lack of OpenTelemetry on Python-3.7

* Update tests

* Fix more test assertions

* Updates from code review

* Update tests with code review suggestions

* Remove return per code review nit
@odeke-em
Copy link
Contributor Author

odeke-em commented Jun 5, 2025

Was split up but can be revived if needed.

@odeke-em odeke-em closed this Jun 5, 2025
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.

5 participants