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鈥檒l 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 a review from as a code owner Dec 21, 2020
@tswast tswast requested review from steffnay and removed request for Dec 21, 2020
@google-cla google-cla bot added the cla: yes label Dec 21, 2020
@tswast
Copy link
Contributor Author

@tswast tswast commented Dec 21, 2020

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

Loading

@tswast tswast requested a review from shollyman Jan 6, 2021
Copy link
Contributor

@plamut plamut left a comment

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.

Loading

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

@plamut plamut Jan 7, 2021

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)

Loading

Copy link
Contributor Author

@tswast tswast Jan 11, 2021

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.

Loading

@tswast tswast merged commit 7ea6b7c into googleapis:master Jan 11, 2021
10 of 11 checks passed
Loading
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.

4 participants