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(observability): more descriptive and value adding spans #1241

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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
google/cloud/spanner_v1/database.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/database.py Show resolved Hide resolved
google/cloud/spanner_v1/database.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/database.py Show resolved Hide resolved
google/cloud/spanner_v1/database.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/_opentelemetry_tracing.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/snapshot.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/transaction.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/transaction.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/session.py Outdated Show resolved Hide resolved
@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
google/cloud/spanner_v1/_opentelemetry_tracing.py Outdated Show resolved Hide resolved
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.

google/cloud/spanner_v1/database.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/pool.py Outdated Show resolved Hide resolved
@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.
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