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

fix: use debug logging level for OpenTelemetry message - #442

Merged
tswast merged 5 commits into
googleapis:masterfrom
tswast:issue441-telemetry-warning
Jan 11, 2021
Merged

fix: use debug logging level for OpenTelemetry message#442
tswast merged 5 commits into
googleapis:masterfrom
tswast:issue441-telemetry-warning

Conversation

@tswast

Copy link
Copy Markdown
Contributor

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 teamDecember 21, 2020 15:26
@google-clagoogle-claBot added the cla: yes This human has signed the Contributor License Agreement. label Dec 21, 2020
@product-auto-labelproduct-auto-labelBot added the api: bigquery Issues related to the googleapis/python-bigquery API. label Dec 21, 2020
@tswast

Copy link
Copy Markdown
ContributorAuthor

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

@tswast
tswast requested a review from shollymanJanuary 6, 2021 23:05

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

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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.

@tswasttswast 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:masterJan 11, 2021
@gcf-merge-on-greengcf-merge-on-greenBot removed the automerge Merge the pull request once unit tests and other checks pass. label Jan 11, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

api: bigqueryIssues related to the googleapis/python-bigquery API.cla: yesThis 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

@tswast@shollyman@plamut@mik-laj