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.

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

feat: add opentelemetry tracing #215

Merged
merged 45 commits into from Aug 21, 2020
Merged

Conversation

@aravinsiva
Copy link
Contributor

@aravinsiva aravinsiva commented Aug 9, 2020

PR is to add Opentelemetry instrumentations to all api call's made by client.py and job.py modules.
This implementation was based on the Go implementation and was designed very similarly to the pub/sub and spanner implementation. Which can be found here:
googleapis/python-spanner@4069c37
googleapis/python-pubsub#149
googleapis/python-pubsub#149

Please note this PR covers the addition of instrumentation and testing of the tracing module. Another PR will be made to address additional tests being added to the test_client.py and test_job.py modules.

Addresses:
#220 & #221

@google-cla google-cla bot added the cla: yes label Aug 9, 2020
@aravinsiva aravinsiva changed the title Opentelemetry tracing feat: add opentelemetry tracing Aug 9, 2020
@aravinsiva aravinsiva marked this pull request as ready for review Aug 10, 2020
README.rst Outdated Show resolved Hide resolved
Loading
README.rst Outdated Show resolved Hide resolved
Loading
README.rst Outdated Show resolved Hide resolved
Loading
README.rst Outdated Show resolved Hide resolved
Loading
README.rst Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/job.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/job.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/opentelemetry_tracing.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/opentelemetry_tracing.py Outdated Show resolved Hide resolved
Loading
noxfile.py Outdated Show resolved Hide resolved
Loading
README.rst Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/client.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/client.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/client.py Outdated Show resolved Hide resolved
Loading
Copy link
Member

@kintel kintel left a comment

I've added a handful of comments/questions.

One open question: You have a unit test for the SpanCreator class, but the individual API calls aren't tested for instrumentation. I'm not sure what the testing strategy is for this repo, but such tests would be nice to have.

Loading

README.rst Outdated Show resolved Hide resolved
Loading
README.rst Outdated Show resolved Hide resolved
Loading
README.rst Outdated Show resolved Hide resolved
Loading
Loading
google/cloud/bigquery/opentelemetry_tracing.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/opentelemetry_tracing.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/opentelemetry_tracing.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/opentelemetry_tracing.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/job.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/opentelemetry_tracing.py Outdated Show resolved Hide resolved
Loading
@aravinsiva
Copy link
Contributor Author

@aravinsiva aravinsiva commented Aug 11, 2020

I've added a handful of comments/questions.

One open question: You have a unit test for the SpanCreator class, but the individual API calls aren't tested for instrumentation. I'm not sure what the testing strategy is for this repo, but such tests would be nice to have.

Will add these tests unless @tswast has any objections.

Loading

google/cloud/bigquery/opentelemetry_tracing.py Outdated Show resolved Hide resolved
Loading
Loading
noxfile.py Outdated Show resolved Hide resolved
Loading
setup.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/client.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/job.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/opentelemetry_tracing.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/opentelemetry_tracing.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/opentelemetry_tracing.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/opentelemetry_tracing.py Outdated Show resolved Hide resolved
Loading
tests/unit/test_client.py Outdated Show resolved Hide resolved
Loading
tests/unit/test_opentelemetry_tracing.py Outdated Show resolved Hide resolved
Loading
README.rst Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/client.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/opentelemetry_tracing.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/opentelemetry_tracing.py Outdated Show resolved Hide resolved
Loading
Loading
tests/unit/test_client.py Outdated Show resolved Hide resolved
Loading
tests/unit/test_client.py Outdated Show resolved Hide resolved
Loading
tests/unit/test_client.py Outdated Show resolved Hide resolved
Loading
"db.name": "test_project",
"location": "test_location",
}
with unittest.TestCase().assertRaises(GoogleAPICallError):
Copy link
Contributor

@tswast tswast Aug 20, 2020

Choose a reason for hiding this comment

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

Could you also check that the status code is set on the span?

Loading

Copy link
Contributor Author

@aravinsiva aravinsiva Aug 20, 2020

Choose a reason for hiding this comment

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

This test just asserts the exception was thrown there is another test that I wrote that verify's span status. Seen here:
@unittest.skipIf(opentelemetry is None, "Requiresopentelemetry")
def test_span_status_is_set(self):
from google.cloud.bigquery.routine import Routine

(line 1255 of test_client.py)

Loading

README.rst Outdated

.. code-block:: console

pip install opentelemetry-api opentelemetry-sdk opentelemetry-instrumentation opentelemetry-exporter-google-cloud
Copy link
Contributor

@tswast tswast Aug 20, 2020

Choose a reason for hiding this comment

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

I'd still prefer if these dependencies were defined in extras. You can add a line to exclude from all since it's not supported on Python 2.7.

Loading

noxfile.py Outdated Show resolved Hide resolved
Loading
tests/unit/test_client.py Outdated Show resolved Hide resolved
Loading
tests/unit/test_client.py Outdated Show resolved Hide resolved
Loading
tests/unit/test_client.py Outdated Show resolved Hide resolved
Loading
tests/unit/test_client.py Outdated Show resolved Hide resolved
Loading
tests/unit/test_client.py Outdated Show resolved Hide resolved
Loading
kintel
kintel approved these changes Aug 20, 2020
Copy link
Member

@kintel kintel left a comment

Looks good from my perspective!

Loading

tswast
tswast approved these changes Aug 20, 2020
Copy link
Contributor

@tswast tswast left a comment

Just one nit, but otherwise looks good!

Loading

setup.py Outdated Show resolved Hide resolved
Loading
tswast
tswast approved these changes Aug 20, 2020
tests/unit/test_opentelemetry_tracing.py Outdated Show resolved Hide resolved
Loading
tests/unit/test_opentelemetry_tracing.py Outdated Show resolved Hide resolved
Loading
tests/unit/test_opentelemetry_tracing.py Outdated Show resolved Hide resolved
Loading
tswast
tswast approved these changes Aug 21, 2020
@tswast tswast merged commit a04996c into googleapis:master Aug 21, 2020
10 checks passed
Loading
gcf-merge-on-green bot pushed a commit that referenced this issue Sep 22, 2020
🤖 I have created a release \*beep\* \*boop\* 
---
## [1.28.0](https://www.github.com/googleapis/python-bigquery/compare/v1.27.2...v1.28.0) (2020-09-22)


### Features

* add custom cell magic parser to handle complex `--params` values ([#213](https://www.github.com/googleapis/python-bigquery/issues/213)) ([dcfbac2](https://www.github.com/googleapis/python-bigquery/commit/dcfbac267fbf66d189b0cc7e76f4712122a74b7b))
* add instrumentation to list methods ([#239](https://www.github.com/googleapis/python-bigquery/issues/239)) ([fa9f9ca](https://www.github.com/googleapis/python-bigquery/commit/fa9f9ca491c3f9954287102c567ec483aa6151d4))
* add opentelemetry tracing ([#215](https://www.github.com/googleapis/python-bigquery/issues/215)) ([a04996c](https://www.github.com/googleapis/python-bigquery/commit/a04996c537e9d8847411fcbb1b05da5f175b339e))
* expose require_partition_filter for hive_partition ([#257](https://www.github.com/googleapis/python-bigquery/issues/257)) ([aa1613c](https://www.github.com/googleapis/python-bigquery/commit/aa1613c1bf48c7efb999cb8b8c422c80baf1950b))


### Bug Fixes

* fix dependency issue in fastavro ([#241](https://www.github.com/googleapis/python-bigquery/issues/241)) ([2874abf](https://www.github.com/googleapis/python-bigquery/commit/2874abf4827f1ea529519d4b138511d31f732a50))
* update minimum dependency versions ([#263](https://www.github.com/googleapis/python-bigquery/issues/263)) ([1be66ce](https://www.github.com/googleapis/python-bigquery/commit/1be66ce94a32b1f924bdda05d068c2977631af9e))
* validate job_config.source_format in load_table_from_dataframe ([#262](https://www.github.com/googleapis/python-bigquery/issues/262)) ([6160fee](https://www.github.com/googleapis/python-bigquery/commit/6160fee4b1a79b0ea9031cc18caf6322fe4c4084))


### Documentation

* recommend insert_rows_json to avoid call to tables.get ([#258](https://www.github.com/googleapis/python-bigquery/issues/258)) ([ae647eb](https://www.github.com/googleapis/python-bigquery/commit/ae647ebd68deff6e30ca2cffb5b7422c6de4940b))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants