Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

feat: add opentelemetry tracing#215

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
tswast merged 45 commits intogoogleapis:masterfromaravinsiva:opentelemetry-tracing
Aug 21, 2020

Conversation

@aravinsiva
Copy link
Contributor

@aravinsivaaravinsiva commentedAug 9, 2020•
edited
Loading

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: yesThis human has signed the Contributor License Agreement. labelAug 9, 2020
@aravinsivaaravinsiva changed the titleOpentelemetry tracingfeat: add opentelemetry tracingAug 9, 2020
@aravinsivaaravinsiva marked this pull request as ready for reviewAugust 10, 2020 13:28
@tswasttswast added the kokoro:runAdd this label to force Kokoro to re-run the tests. labelAug 10, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:runAdd this label to force Kokoro to re-run the tests. labelAug 10, 2020
@tswasttswast added the kokoro:runAdd this label to force Kokoro to re-run the tests. labelAug 11, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:runAdd this label to force Kokoro to re-run the tests. labelAug 11, 2020
Copy link

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

@aravinsiva
Copy link
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.

@tswasttswast added the kokoro:runAdd this label to force Kokoro to re-run the tests. labelAug 11, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:runAdd this label to force Kokoro to re-run the tests. labelAug 11, 2020
@tswasttswast added the kokoro:runAdd this label to force Kokoro to re-run the tests. labelAug 20, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:runAdd this label to force Kokoro to re-run the tests. labelAug 20, 2020
@tswasttswast added the kokoro:runAdd this label to force Kokoro to re-run the tests. labelAug 20, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:runAdd this label to force Kokoro to re-run the tests. labelAug 20, 2020
"db.name":"test_project",
"location":"test_location",
}
withunittest.TestCase().assertRaises(GoogleAPICallError):
Copy link
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?

Copy link
ContributorAuthor

@aravinsivaaravinsivaAug 20, 2020•
edited
Loading

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)

README.rst Outdated

..code-block::console
pip install opentelemetry-api opentelemetry-sdk opentelemetry-instrumentation opentelemetry-exporter-google-cloud
Copy link
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 inextras. You can add a line to exclude fromall since it's not supported on Python 2.7.

Copy link

@kintelkintel left a comment

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:runAdd this label to force Kokoro to re-run the tests. labelAug 20, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:runAdd this label to force Kokoro to re-run the tests. labelAug 20, 2020
Copy link
Contributor

@tswasttswast left a comment

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!

@tswasttswast added the kokoro:runAdd this label to force Kokoro to re-run the tests. labelAug 20, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:runAdd this label to force Kokoro to re-run the tests. labelAug 20, 2020
@tswasttswast added the kokoro:runAdd this label to force Kokoro to re-run the tests. labelAug 20, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:runAdd this label to force Kokoro to re-run the tests. labelAug 20, 2020
@product-auto-labelproduct-auto-labelbot added the api: bigqueryIssues related to the googleapis/python-bigquery API. labelAug 21, 2020
@tswasttswast added the kokoro:runAdd this label to force Kokoro to re-run the tests. labelAug 21, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:runAdd this label to force Kokoro to re-run the tests. labelAug 21, 2020
@tswasttswast merged commita04996c intogoogleapis:masterAug 21, 2020
gcf-merge-on-greenbot pushed a commit that referenced this pull requestSep 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 freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@tswasttswasttswast approved these changes

+3 more reviewers

@AndrewAXueAndrewAXueAndrewAXue left review comments

@kintelkintelkintel 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
[8]ページ先頭
©2009-2025 Movatter.jp