chore: Fallback to inputs if impersonation fails#52052
Conversation
| sensitive_config = {} | ||
| is_impersonated = True | ||
| if isinstance(private_key, str) and isinstance(private_key_id, str) and isinstance(token_uri, str): | ||
| sensitive_config["private_key"] = private_key | ||
| sensitive_config["private_key_id"] = private_key_id | ||
| sensitive_config["token_uri"] = token_uri | ||
|
|
||
| is_impersonated = False | ||
|
|
There was a problem hiding this comment.
Now this is closer to where is_impersonated is used
| # We cannot fallback to using inputs | ||
| raise |
There was a problem hiding this comment.
This is the case when impersonation is required: i.e. new batch exports using impersonation. Everyone else has inputs.
Prompt To Fix All With AIThis is a comment left during a code review.
Path: products/batch_exports/backend/temporal/destinations/bigquery_batch_export.py
Line: 1343-1344
Comment:
**ERROR-level log fires even when fallback succeeds**
`LOGGER.exception(...)` logs at ERROR level and includes the full exception traceback. When the fallback to `from_service_account_inputs` succeeds, the operation completes successfully — but the ERROR log (and any associated alert) still fires. This creates noisy false-positive errors in monitoring dashboards.
Consider logging at WARNING before trying the fallback, and only escalate to ERROR if the fallback also fails:
```python
except Exception:
LOGGER.warning("Initialize client from service account integration failed, falling back to inputs", exc_info=True)
# TODO: Migrate everyone and remove this
if (
inputs.private_key is None
or inputs.private_key_id is None
or inputs.token_uri is None
or inputs.client_email is None
):
# We cannot fallback to using inputs
LOGGER.exception("Cannot fall back: missing required inputs")
raise
bq_client = BigQueryClient.from_service_account_inputs(
inputs.private_key, inputs.private_key_id, inputs.token_uri, inputs.client_email, project_id
)
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "chore: Re-order statements a bit" | Re-trigger Greptile |
| except Exception: | ||
| LOGGER.exception("Initialize client from service account failed") |
There was a problem hiding this comment.
ERROR-level log fires even when fallback succeeds
LOGGER.exception(...) logs at ERROR level and includes the full exception traceback. When the fallback to from_service_account_inputs succeeds, the operation completes successfully — but the ERROR log (and any associated alert) still fires. This creates noisy false-positive errors in monitoring dashboards.
Consider logging at WARNING before trying the fallback, and only escalate to ERROR if the fallback also fails:
except Exception:
LOGGER.warning("Initialize client from service account integration failed, falling back to inputs", exc_info=True)
# TODO: Migrate everyone and remove this
if (
inputs.private_key is None
or inputs.private_key_id is None
or inputs.token_uri is None
or inputs.client_email is None
):
# We cannot fallback to using inputs
LOGGER.exception("Cannot fall back: missing required inputs")
raise
bq_client = BigQueryClient.from_service_account_inputs(
inputs.private_key, inputs.private_key_id, inputs.token_uri, inputs.client_email, project_id
)Prompt To Fix With AI
This is a comment left during a code review.
Path: products/batch_exports/backend/temporal/destinations/bigquery_batch_export.py
Line: 1343-1344
Comment:
**ERROR-level log fires even when fallback succeeds**
`LOGGER.exception(...)` logs at ERROR level and includes the full exception traceback. When the fallback to `from_service_account_inputs` succeeds, the operation completes successfully — but the ERROR log (and any associated alert) still fires. This creates noisy false-positive errors in monitoring dashboards.
Consider logging at WARNING before trying the fallback, and only escalate to ERROR if the fallback also fails:
```python
except Exception:
LOGGER.warning("Initialize client from service account integration failed, falling back to inputs", exc_info=True)
# TODO: Migrate everyone and remove this
if (
inputs.private_key is None
or inputs.private_key_id is None
or inputs.token_uri is None
or inputs.client_email is None
):
# We cannot fallback to using inputs
LOGGER.exception("Cannot fall back: missing required inputs")
raise
bq_client = BigQueryClient.from_service_account_inputs(
inputs.private_key, inputs.private_key_id, inputs.token_uri, inputs.client_email, project_id
)
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
it's not a false positive: failure is not expected we want to be alerted. The fallback is there to make things safer, not because it is part of the expected path. The "TODO: Remove this" comment should have made this obvious.
Problem
Migrating everyone to new integration is scary, let's make it safer.
Changes
If using the new methods fails, fallback to using inputs.
Also re-ordered things a bit in the integration model, but no actual changes.
How did you test this code?
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Publish to changelog?
Docs update