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

cnnradams · 2020-07-02T14:17:06Z

Creates spans around batch/snapshot/transaction calls with relevant information that could help debug latency issues + provide insight into spanner calls
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 :)

larkee

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

google/cloud/spanner_v1/_opentelemetry_tracing.py

google/cloud/spanner_v1/transaction.py

tests/system/test_system.py

tests/_helpers.py

tests/unit/test_session.py

google/cloud/spanner_v1/snapshot.py

tests/unit/test__opentelemetry_tracing.py

larkee · 2020-07-15T08:29:07Z

noxfile.py

    default(session)


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


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?

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

@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.

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.

google/cloud/spanner_v1/_opentelemetry_tracing.py

tests/_helpers.py

hengfengli

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.

docs/opentelemetry-tracing.rst

google/cloud/spanner_v1/_opentelemetry_tracing.py

google/cloud/spanner_v1/snapshot.py

google/cloud/spanner_v1/transaction.py

tests/_helpers.py

tests/system/test_system.py

tests/unit/test__opentelemetry_tracing.py

cnnradams · 2020-07-27T13:41:17Z

what would it take to get this PR moving again?

hengfengli · 2020-07-28T00:22:40Z

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

larkee

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

docs/opentelemetry-tracing.rst

hengfengli

LGTM.

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 requested changes Jul 15, 2020

View reviewed changes

cnnradams added 3 commits July 15, 2020 17:14

feat: add optional span creation with OpenTelemetry

2a07883

bring back support for python2.7

70a9012

address comments

bee0e58

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

tests/_helpers.py Outdated Show resolved Hide resolved

fix 2.7 tests

027e0b1

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 requested changes Jul 18, 2020

View reviewed changes

cnnradams force-pushed the opentelemetry-tracing branch from 25a6ec0 to 0ad6be0 Compare July 20, 2020 14:59

nit fixes

de4b7e7

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

db.statement join with ;

190f6a7

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

larkee approved these changes Jul 28, 2020

View reviewed changes

docs/opentelemetry-tracing.rst Outdated Show resolved Hide resolved

hengfengli approved these changes Jul 28, 2020

View reviewed changes

Update docs/opentelemetry-tracing.rst

e78e780

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

feat: Add support for open telemetry googleapis/python-spanner-django#633

Merged

Conversation

cnnradams commented Jul 2, 2020

Uh oh!

larkee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

larkee Jul 15, 2020

Choose a reason for hiding this comment

Uh oh!

cnnradams Jul 15, 2020

Choose a reason for hiding this comment

Uh oh!

hengfengli Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

larkee Jul 21, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hengfengli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cnnradams commented Jul 27, 2020

Uh oh!

hengfengli commented Jul 28, 2020

Uh oh!

larkee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hengfengli left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants