MINOR: Looker error handling improvements#27236
Conversation
Code Review ✅ ApprovedLooker error handling improvements implemented with clean, focused changes. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
There was a problem hiding this comment.
Pull request overview
Improves Looker ingestion error handling by avoiding noisy errors for missing LookML views and preventing credential leakage in clone failures; also hardens FQN parsing for multi-part table names.
Changes:
- Treat missing LookML views during
_process_viewas a warning (no emittedStackTraceError). - Sanitize git clone error logs to avoid leaking embedded tokens/PATs.
- Fix
split_table_nameto safely handle 4+ part table names by using the last 3 segments, with added unit tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ingestion/src/metadata/ingestion/source/dashboard/looker/metadata.py | Logs a warning instead of yielding an error when a view can’t be found in configured repos. |
| ingestion/src/metadata/ingestion/source/dashboard/looker/utils.py | Sanitizes clone exception messages to mask credentials before logging. |
| ingestion/src/metadata/utils/fqn.py | Makes split_table_name robust for 4+ part names by truncating to the last 3 parts. |
| ingestion/tests/unit/topology/dashboard/test_looker.py | Adds unit test ensuring missing views don’t produce yielded errors. |
| ingestion/tests/unit/topology/dashboard/test_looker_utils.py | Adds unit tests ensuring clone failures don’t leak credentials in logs. |
| ingestion/tests/unit/test_fqn.py | Adds unit tests covering split_table_name behavior for 1–5 part names. |
|
| logger.info(f"repo {repo_name} cloned to {path}") | ||
| except Exception as exc: | ||
| logger.error(f"GitHubCloneReader::_clone: ERROR {exc} ") | ||
| sanitized_msg = re.sub(r"https://[^@]+@", "https://****@", str(exc)) |
There was a problem hiding this comment.
should this be a generic util someplace else to clean URIs?
| # Pad None to the left until size of list is 3 | ||
| full_details: List[Optional[str]] = ([None] * (3 - len(details))) + details | ||
| # If more than 3 parts, take only the last 3 (database, schema, table) | ||
| full_details: List[Optional[str]] = ([None] * max(0, 3 - len(details))) + details[ |
There was a problem hiding this comment.
this is sort of an edge case for looker where reads from a information schema table from bigquery project-name.region-name.INFORMATION_SCHEMA.table_name and it fails with error too many values to unpack in the next line
handling this here as it will also automatically solve similar error we would face in bigquery lineage
There was a problem hiding this comment.
ok I might add this as a comment in the code so we don't forget + test
There was a problem hiding this comment.
already added tests, I'll add the comment as well
🟡 Playwright Results — all passed (27 flaky)✅ 3596 passed · ❌ 0 failed · 🟡 27 flaky · ⏭️ 207 skipped
🟡 27 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
- Extract URL credential sanitization to generic `sanitize_url_credentials` in logger utils - Fix misleading log prefix `GitHubCloneReader::_clone` → `_clone_repo` - Add BigQuery INFORMATION_SCHEMA context to `split_table_name` comment - Add unit tests for `sanitize_url_credentials` Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>



Describe your changes:
Fixes
I worked on ... because ...
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>