-
Notifications
You must be signed in to change notification settings - Fork 88
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
observability: annotate Session+SessionPool events #1207
base: main
Are you sure you want to change the base?
Conversation
8d6f2b7
to
4bc937e
Compare
4bc937e
to
0d5bf26
Compare
0d5bf26
to
3617921
Compare
f71d05c
to
eb9dd5a
Compare
b1ea772
to
ab23f09
Compare
Kindly cc-ing you @harshachinta. |
google/cloud/spanner_v1/pool.py
Outdated
start_time = time.time() | ||
current_span = get_current_span() | ||
current_span.add_event( | ||
"Waiting for a session to become available", {"kind": "pinging_pool"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this not "Acquiring a session"? Lets maintain span consistency across all the pools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because really it is waiting for a session to become available per
python-spanner/google/cloud/spanner_v1/pool.py
Lines 218 to 238 in b2e6762
def get(self, timeout=None): | |
"""Check a session out from the pool. | |
:type timeout: int | |
:param timeout: seconds to block waiting for an available session | |
:rtype: :class:`~google.cloud.spanner_v1.session.Session` | |
:returns: an existing session from the pool, or a newly-created | |
session. | |
:raises: :exc:`queue.Empty` if the queue is empty. | |
""" | |
if timeout is None: | |
timeout = self.default_timeout | |
session = self._sessions.get(block=True, timeout=timeout) | |
if not session.exists(): | |
session = self._database.session() | |
session.create() | |
return session |
and in our document requirements there is a clear need to add that case
Kindly please take another look @harshachinta, feedback addressed, thank you for the code review! |
google/cloud/spanner_v1/pool.py
Outdated
) | ||
|
||
if requested_session_count > 0: | ||
current_span.add_event( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
current_span is not initialized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sheesh, sorry I had a bunch of benchmarks running locally and they interrupted my nox -s unit-3.8
tests so didn't see those but all fixed now.
73250ca
to
0379012
Compare
@harshachinta @surbhigarg92 @sakthivelmanii kindly help me trigger the bots for this PR too. Thank you. |
This change adds annotations for session and session pool events to aid customers in debugging latency issues with session pool malevolence and also for maintainers to figure out which session pool type is the most appropriate. Updates googleapis#1170
This allows us to avoid the flakes with OpenTelemetry's inMemorySpanExporter that might not have received the exported spans yet.
ef7052e
to
2d4cbca
Compare
2d4cbca
to
47d6e17
Compare
This PR could use some bots runs. Thank you! |
1 similar comment
This PR could use some bots runs. Thank you! |
This change adds annotations for session and session pool events to aid customers in debugging latency issues with session pool malevolence and also for maintainers to figure out which session pool type is the most appropriate.
Updates #1170