Skip to content
This repository was archived by the owner on Mar 6, 2026. It is now read-only.

feat: add opentelemetry tracing - #215

Merged
tswast merged 45 commits into
googleapis:masterfrom
aravinsiva:opentelemetry-tracing
Aug 21, 2020
Merged

feat: add opentelemetry tracing#215
tswast merged 45 commits into
googleapis:masterfrom
aravinsiva:opentelemetry-tracing

Conversation

@aravinsiva

@aravinsivaaravinsiva commented Aug 9, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

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-clagoogle-claBot added the cla: yes This human has signed the Contributor License Agreement. label Aug 9, 2020
@aravinsivaaravinsiva changed the title Opentelemetry tracingfeat: add opentelemetry tracingAug 9, 2020
@aravinsiva
aravinsiva marked this pull request as ready for review August 10, 2020 13:28
@tswasttswast added the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 10, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 10, 2020
tswast
tswast reviewed Aug 10, 2020
View reviewed changes
Comment threadREADME.rst Outdated
Comment threadREADME.rst Outdated
Comment threadREADME.rst Outdated
Comment threadREADME.rst Outdated
Comment threadREADME.rst Outdated
Comment threadgoogle/cloud/bigquery/job.py Outdated
Comment threadgoogle/cloud/bigquery/job.py Outdated
Comment threadgoogle/cloud/bigquery/opentelemetry_tracing.py Outdated
Comment threadgoogle/cloud/bigquery/opentelemetry_tracing.py Outdated
Comment threadnoxfile.py Outdated
@tswast

tswast commented Aug 10, 2020

Copy link
Copy Markdown
Contributor

Build is breaking:

https://source.cloud.google.com/results/invocations/6578f68f-1263-4029-ac27-22c033fe6d86/targets/cloud-devrel%2Fclient-libraries%2Fpython%2Fgoogleapis%2Fpython-bigquery%2Fdocs%2Fdocs-presubmit/log

sjbrunst
sjbrunst reviewed Aug 11, 2020
View reviewed changes
Comment threadREADME.rst Outdated
Comment threadgoogle/cloud/bigquery/client.py Outdated
Comment threadgoogle/cloud/bigquery/client.py Outdated
Comment threadgoogle/cloud/bigquery/client.py Outdated
@tswasttswast added the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 11, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 11, 2020
kintel
kintel suggested changes Aug 11, 2020
View reviewed changes

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

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.

Comment threadREADME.rst Outdated
Comment threadREADME.rst Outdated
Comment threadREADME.rst Outdated
Comment threadgoogle/cloud/bigquery/opentelemetry_tracing.py
Comment threadgoogle/cloud/bigquery/opentelemetry_tracing.py Outdated
Comment threadgoogle/cloud/bigquery/opentelemetry_tracing.py Outdated
Comment threadgoogle/cloud/bigquery/opentelemetry_tracing.py Outdated
Comment threadgoogle/cloud/bigquery/opentelemetry_tracing.py Outdated
Comment threadgoogle/cloud/bigquery/job.py Outdated
Comment threadgoogle/cloud/bigquery/opentelemetry_tracing.py Outdated
@aravinsiva

aravinsiva commented Aug 11, 2020

Copy link
Copy Markdown
ContributorAuthor

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.

kintel
kintel reviewed Aug 11, 2020
View reviewed changes
Comment threadgoogle/cloud/bigquery/opentelemetry_tracing.py Outdated
tswast
tswast reviewed Aug 11, 2020
View reviewed changes
Comment threadgoogle/cloud/bigquery/opentelemetry_tracing.py
Comment threadnoxfile.py Outdated
Comment threadsetup.py Outdated
@tswasttswast added the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 11, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 11, 2020
sjbrunst
sjbrunst approved these changes Aug 20, 2020
View reviewed changes
Comment threadREADME.rst Outdated
@tswasttswast added the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 20, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 20, 2020
tswast
tswast reviewed Aug 20, 2020
View reviewed changes
Comment threadgoogle/cloud/bigquery/client.py Outdated
Comment threadgoogle/cloud/bigquery/opentelemetry_tracing.py Outdated
Comment threadgoogle/cloud/bigquery/opentelemetry_tracing.py Outdated
Comment threadgoogle/cloud/bigquery/opentelemetry_tracing.py
Comment threadtests/unit/test_client.py Outdated
Comment threadtests/unit/test_client.py Outdated
Comment threadtests/unit/test_client.py Outdated
@tswasttswast added the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 20, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 20, 2020
tswast
tswast reviewed Aug 20, 2020
View reviewed changes
Comment threadtests/unit/test_opentelemetry_tracing.py Outdated
"db.name": "test_project",
"location": "test_location",
}
with unittest.TestCase().assertRaises(GoogleAPICallError):

@tswasttswastAug 20, 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.

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

@aravinsivaaravinsivaAug 20, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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)

Comment threadREADME.rst Outdated

.. code-block:: console

pip install opentelemetry-api opentelemetry-sdk opentelemetry-instrumentation opentelemetry-exporter-google-cloud

@tswasttswastAug 20, 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.

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.

Comment threadnoxfile.py Outdated
Comment threadtests/unit/test_client.py Outdated
Comment threadtests/unit/test_client.py Outdated
Comment threadtests/unit/test_client.py Outdated
Comment threadtests/unit/test_client.py Outdated
Comment threadtests/unit/test_client.py Outdated
kintel
kintel approved these changes Aug 20, 2020
View reviewed changes

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

Looks good from my perspective!

@tswasttswast added the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 20, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 20, 2020
tswast
tswast approved these changes Aug 20, 2020
View reviewed changes

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

Just one nit, but otherwise looks good!

Comment threadsetup.py Outdated
@tswasttswast added the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 20, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 20, 2020
@tswasttswast added the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 20, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 20, 2020
@product-auto-labelproduct-auto-labelBot added the api: bigquery Issues related to the googleapis/python-bigquery API. label Aug 21, 2020
tswast
tswast reviewed Aug 21, 2020
View reviewed changes
Comment threadtests/unit/test_opentelemetry_tracing.py Outdated
Comment threadtests/unit/test_opentelemetry_tracing.py
Comment threadtests/unit/test_opentelemetry_tracing.py Outdated
@tswasttswast added the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 21, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 21, 2020
@tswast
tswast merged commit a04996c into googleapis:masterAug 21, 2020
gcf-merge-on-greenBot pushed a commit that referenced this pull request Sep 22, 2020
@release-please
chore: release 1.28.0 (#238)
c75e9cd
🤖 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).
@release-pleaserelease-pleaseBot mentioned this pull request Jan 4, 2022
Closed
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

4 more reviewers
@AndrewAXueAndrewAXueAndrewAXue left review comments
@kintelkintelkintel approved these changes
@tswasttswasttswast approved these changes
@sjbrunstsjbrunstsjbrunst approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: bigqueryIssues related to the googleapis/python-bigquery API.cla: yesThis human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@aravinsiva@tswast@kintel@sjbrunst@AndrewAXue@yoshi-kokoro