| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
@sakthivelmanii @harshachinta kindly help me run the bots. Thank you |
Sorry, something went wrong.
| yield None | ||
| return | ||
|
|
||
| if session: |
There was a problem hiding this comment.
@odeke-em
We should move this latest change of updating session last use time to beginning of this function, otherwise when opentlemetry is not installed then that update will never happen.
Sorry, something went wrong.
There was a problem hiding this comment.
@odeke-em
this is not yet addresses.
Sorry, something went wrong.
There was a problem hiding this comment.
I removed that @harshachinta as it is from the bigger PR and I don't think we shall need it anymore once some tests are sent in the other PR. I deleted it.
Sorry, something went wrong.
There was a problem hiding this comment.
It is not from your PR, this was added by us to handle one case. Don't remove that
I am talking about this code. We need this to know when was a session last used to avoid making a backend call to verify session existence.
if session:
session._last_use_time = datetime.now()
Sorry, something went wrong.
| :param values: Values to be modified. | ||
| """ | ||
| self._mutations.append(Mutation(insert=_make_write_pb(table, columns, values))) | ||
| add_event_on_current_span( |
There was a problem hiding this comment.
I understand this requirement was bought out when we met last time.
Thinking out loud.
Cloud spanner has a mutation limit of 40000 in one request.
So lets say a customer adds mutations in a loop, then this will print 40,000 events on the span.
Is this a good user experience? Also this could add up to the storage cost on customer end?
Sorry, something went wrong.
There was a problem hiding this comment.
That's a great question but it won't be a problem because:
What we can do is perhaps document how to change span event limits and also document that we add events for span mutations.
Sorry, something went wrong.
There was a problem hiding this comment.
@sakthivelmanii
I’d like to get your thoughts on this.
In my opinion, adding a separate event for every mutation could clutter the trace spans with too many events. Instead, I’d suggest avoiding one event per mutation. We already track the num_mutations attribute in the Commit request (here), which effectively provides customers with insights into the number of mutations in a commit.
Let me know what you think.
Sorry, something went wrong.
| "CloudSpanner.CreateSession", | ||
| "CloudSpanner.Transaction.execute_streaming_sql", | ||
| "CloudSpanner.Transaction.execute_streaming_sql", | ||
| "CloudSpanner.Transaction.commit", | ||
| "CloudSpanner.Session.run_in_transaction", | ||
| "CloudSpanner.Database.run_in_transaction", |
There was a problem hiding this comment.
There is some mistake in ordering here. Can you check?
Ideally the order should be,
"CloudSpanner.Database.run_in_transaction",
"CloudSpanner.CreateSession",
"CloudSpanner.Session.run_in_transaction",
"CloudSpanner.Transaction.execute_streaming_sql",
"CloudSpanner.Transaction.execute_streaming_sql",
"CloudSpanner.Transaction.commit",
```
Sorry, something went wrong.
There was a problem hiding this comment.
It isn't a mistake though but rather the order in which spans are ended @harshachinta.
Sorry, something went wrong.
| :param values: Values to be modified. | ||
| """ | ||
| self._mutations.append(Mutation(insert=_make_write_pb(table, columns, values))) | ||
| add_event_on_current_span( |
There was a problem hiding this comment.
@sakthivelmanii
I’d like to get your thoughts on this.
In my opinion, adding a separate event for every mutation could clutter the trace spans with too many events. Instead, I’d suggest avoiding one event per mutation. We already track the num_mutations attribute in the Commit request (here), which effectively provides customers with insights into the number of mutations in a commit.
Let me know what you think.
Sorry, something went wrong.
| "exception", | ||
| { | ||
| "exception.type": "IndexError", | ||
| "exception.message": "pop from empty list", | ||
| "exception.stacktrace": "EPHEMERAL", | ||
| "exception.escaped": "False", | ||
| }, |
There was a problem hiding this comment.
why is there a repeation of exception event?
Sorry, something went wrong.
There was a problem hiding this comment.
So given that we have a direct invocation of with trace_call("testBind") per
try:
with trace_call("testBind", fauxSession):
pool.bind(database)
except Exception:
pass that then invokes code internally where an exception happens. With the exception re-raised and unhandled until the highest parent, OpenTelemetry records the exception even on the parent. If we swallowed the exception internally we wouldn't
Sorry, something went wrong.
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
…ble + address test feedback
Referencing issue #1269, this update removes adding a span event per mutation, in favour of a future TODO.
* 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 #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
| Back | FazBrowse Home | New Git URL |
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:
Also added important spans: