FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat: add OpenTelemetry tracing to spanner calls by cnnradams · Pull Request #107 · googleapis/python-spanner · GitHub

This repository was archived by the owner on Jun 8, 2026. It is now read-only.
/ python-spanner Public archive

feat: add OpenTelemetry tracing to spanner calls - #107

Merged
larkee merged 7 commits into
googleapis:masterfrom
cnnradams:opentelemetry-tracing
Jul 29, 2020
ConversationCommits (7)ChecksFiles changed
Merged

feat: add OpenTelemetry tracing to spanner calls#107
larkee merged 7 commits into
googleapis:masterfrom
cnnradams:opentelemetry-tracing

Conversation

cnnradams commented Jul 2, 2020

Copy link
Copy Markdown
Contributor
  1. Creates spans around batch/snapshot/transaction calls with relevant information that could help debug latency issues + provide insight into spanner calls
  2. Removed python2.7 from unit/system tests (since OpenTelemetry does not support 2.7). However since OT is an optional dependency with some extra effort the tests could keep using 2.7, is this desired?

Pool/gRPC metrics are also on my TODO list, they will come in a seperate PR once some issues with the OpenTelemetry Metrics SDK are worked out :)

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 2, 2020
cnnradams force-pushed the opentelemetry-tracing branch from 013f11f to 039ff4f Compare July 2, 2020 14:19
larkee added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 6, 2020
yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 6, 2020
cnnradams force-pushed the opentelemetry-tracing branch from 039ff4f to 9b4972e Compare July 6, 2020 19:22
hengfengli requested review from hengfengli and larkee July 15, 2020 01:42
larkee added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jul 15, 2020
yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 15, 2020
larkee suggested changes Jul 15, 2020
View reviewed changes

larkee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I've had a quick look over and I'm happy with how it's looking. Just pointed out some things to address.

Comment thread google/cloud/spanner_v1/_opentelemetry_tracing.py Outdated
Comment thread google/cloud/spanner_v1/transaction.py Outdated
Comment thread tests/system/test_system.py Outdated
Comment thread tests/_helpers.py Outdated
Comment thread tests/unit/test_session.py Outdated
Comment thread google/cloud/spanner_v1/snapshot.py Outdated
Comment thread tests/unit/test__opentelemetry_tracing.py Outdated
Comment thread noxfile.py
default(session)


@nox.session(python=["2.7", "3.7"])

larkee Jul 15, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Removed python2.7 from unit/system tests (since OpenTelemetry does not support 2.7). However since OT is an optional dependency with some extra effort the tests could keep using 2.7, is this desired?

Strictly speaking, Python 2.7 is meant to be deprecated. However, removing it is considered a breaking change. We have a breaking change coming up that we're waiting on. How much work is it to keep using 2.7?

cnnradams Jul 15, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

added back 2.7 support - just had to wrap all the OT tests in a dependency check and not install OT if using 2.7

hengfengli Jul 20, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@larkee Do you have an estimate when will python 2.7 be deprecated?

If we need to merge this first before deprecating 2.7, then we need to remember to remove the dependency checks in OT tests when 2.7 is deprecated.

larkee Jul 21, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Python 2.7 will be deprecated with the microgenerator migration which is currently scheduled for the end of August. I am happy to remove the dependency checks during that migration myself.

Comment thread google/cloud/spanner_v1/_opentelemetry_tracing.py
cnnradams force-pushed the opentelemetry-tracing branch from ef30b12 to bee0e58 Compare July 15, 2020 21:15
larkee added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 16, 2020
yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 16, 2020
larkee reviewed Jul 16, 2020
View reviewed changes
Comment thread tests/_helpers.py Outdated
larkee added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 17, 2020
yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 17, 2020
hengfengli suggested changes Jul 18, 2020
View reviewed changes

hengfengli left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for working on this. This looks great to me. 👍

My major concern is HAS_OPENTELEMETRY_INSTALLED. I think we should get rid of this because it adds a lot of redundant code.

Comment thread docs/opentelemetry-tracing.rst Outdated
Comment thread docs/opentelemetry-tracing.rst Outdated
Comment thread docs/opentelemetry-tracing.rst Outdated
Comment thread google/cloud/spanner_v1/_opentelemetry_tracing.py Outdated
Comment thread google/cloud/spanner_v1/_opentelemetry_tracing.py Outdated
Comment thread google/cloud/spanner_v1/snapshot.py
Comment thread google/cloud/spanner_v1/transaction.py Outdated
Comment thread tests/_helpers.py
Comment thread tests/system/test_system.py Outdated
Comment thread tests/unit/test__opentelemetry_tracing.py
cnnradams force-pushed the opentelemetry-tracing branch from 25a6ec0 to 0ad6be0 Compare July 20, 2020 14:59
cnnradams force-pushed the opentelemetry-tracing branch from 0ad6be0 to de4b7e7 Compare July 20, 2020 15:02
larkee added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 21, 2020
yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 21, 2020
hengfengli added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 22, 2020
yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 22, 2020

cnnradams commented Jul 27, 2020

Copy link
Copy Markdown
Contributor Author

what would it take to get this PR moving again?

hengfengli commented Jul 28, 2020

Copy link
Copy Markdown

@larkee It looks good to me. Can we merge this in?

larkee approved these changes Jul 28, 2020
View reviewed changes

larkee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM with the suggested doc fix 👍
Thanks again for your work and your patience!

Comment thread docs/opentelemetry-tracing.rst Outdated
hengfengli approved these changes Jul 28, 2020
View reviewed changes

hengfengli left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM.

larkee added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 29, 2020
yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 29, 2020
larkee merged commit 4069c37 into googleapis:master Jul 29, 2020
vi3k6i5 mentioned this pull request Jun 16, 2021
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
hengfengli hengfengli approved these changes
larkee larkee approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL