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

fix: use debug logging level for OpenTelemetry message #442

Merged
merged 5 commits into from Jan 11, 2021

Conversation

tswast
Copy link
Contributor

@tswast tswast commented Dec 21, 2020

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #441 🦕

@tswast tswast requested review from a team and steffnay and removed request for a team December 21, 2020 15:26
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 21, 2020
@product-auto-label product-auto-label bot added the api: bigquery Issues related to the googleapis/python-bigquery API. label Dec 21, 2020
@tswast
Copy link
Contributor Author

tswast commented Dec 21, 2020

Updated based on apache/airflow#13131 to not warn at import time.

@tswast tswast requested a review from shollyman January 6, 2021 23:05
Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

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

I agree that the warning should not be issued at import time, but instead when trying to actually use BigQuery (discussion).

DEBUG level is probably indeed better, as opentelemetry is not used by default (it's listed under extras), thus shouldn't bother users that don't choose to install it.

We should choose a different name for the flag, though, IMHO.

"add opentelemetry-api and opentelemetry-instrumentation "
"packages in order to get BigQuery Tracing data."
)
WARNED_OPENTELEMETRY = True
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) It's a bit unexpected to change a "constant", perhaps rename it? Also, since it's an internal state-keeping flag that users should neither know about nor modify it, we should probably mark it as such (e.g. _warned_telemetry)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I've renamed in the latest commit.

@tswast tswast added the automerge Merge the pull request once unit tests and other checks pass. label Jan 11, 2021
@tswast tswast merged commit 7ea6b7c into googleapis:master Jan 11, 2021
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the googleapis/python-bigquery API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Too noise logging about telemetry
4 participants