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.

Sign up for GitHub

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

Jump to bottom

feat: add OpenTelemetry tracing to spanner calls #107

Merged
merged 7 commits into from
Jul 29, 2020
Merged

feat: add OpenTelemetry tracing to spanner calls #107

merged 7 commits into from
Jul 29, 2020

Conversation

cnnradams
Copy link
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 googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 2, 2020
@larkee larkee added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 6, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 6, 2020
@larkee 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 yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 15, 2020
larkee
larkee requested changes Jul 15, 2020
View reviewed changes
Copy link
Contributor

@larkee larkee left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
google/cloud/spanner_v1/transaction.py Outdated Show resolved Hide resolved
tests/system/test_system.py Outdated Show resolved Hide resolved
tests/_helpers.py Outdated Show resolved Hide resolved
tests/unit/test_session.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/snapshot.py Outdated Show resolved Hide resolved
tests/unit/test__opentelemetry_tracing.py Outdated Show resolved Hide resolved
noxfile.py Outdated
def unit(session):
"""Run the unit test suite."""
default(session)


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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
@larkee larkee added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 16, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 16, 2020
larkee
larkee reviewed Jul 16, 2020
View reviewed changes
tests/_helpers.py Outdated Show resolved Hide resolved
@larkee larkee added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 17, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 17, 2020
hengfengli
hengfengli requested changes Jul 18, 2020
View reviewed changes
Copy link

@hengfengli hengfengli left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
docs/opentelemetry-tracing.rst Outdated Show resolved Hide resolved
docs/opentelemetry-tracing.rst Outdated Show resolved Hide resolved
google/cloud/spanner_v1/_opentelemetry_tracing.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/_opentelemetry_tracing.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/snapshot.py Show resolved Hide resolved
google/cloud/spanner_v1/transaction.py Outdated Show resolved Hide resolved
tests/_helpers.py Show resolved Hide resolved
tests/system/test_system.py Outdated Show resolved Hide resolved
tests/unit/test__opentelemetry_tracing.py Show resolved Hide resolved
@larkee larkee added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 21, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 21, 2020
@hengfengli hengfengli added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 22, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 22, 2020
@cnnradams
Copy link
Contributor Author

what would it take to get this PR moving again?

@hengfengli
Copy link

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

larkee
larkee approved these changes Jul 28, 2020
View reviewed changes
Copy link
Contributor

@larkee larkee left a comment

Choose a reason for hiding this comment

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

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

docs/opentelemetry-tracing.rst Outdated Show resolved Hide resolved
hengfengli
hengfengli approved these changes Jul 28, 2020
View reviewed changes
Copy link

@hengfengli hengfengli left a comment

Choose a reason for hiding this comment

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

LGTM.

@larkee larkee added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 29, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 29, 2020
@larkee larkee merged commit 4069c37 into googleapis:master Jul 29, 2020
@vi3k6i5 vi3k6i5 mentioned this pull request Jun 16, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larkee larkee larkee approved these changes

@hengfengli hengfengli hengfengli approved these changes

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
@cnnradams @hengfengli @larkee @googlebot @yoshi-kokoro