Add ClickHouse profiler - #2574
Open
dey-abhishek wants to merge 33 commits into
Open
Conversation
Add ClickHouse as a first-class profiler source, ported from the standalone Field Engineering ClickHouse profiler and reshaped to the Lakebridge profiler contract: - Register 'clickhouse' with an AUTO variant; resolve oss vs cloud from the cloud_mode server setting / *.clickhouse.cloud host (mirrors mssql). - Add ClickHouseConnector (clickhouse-connect HTTP client) and a credential configurator; host-derived secure/port defaults. - Single Python extract step runs 7 read-only system.* collectors (workload, objects, features, dependencies, utilization, security, costs) and writes one DuckDB table per result set; empty result sets create typed stubs from a declared schema catalog so every table always exists. - Cost is actual-billed-cost only via the ClickHouse Cloud usageCost API (no rate card); plan tier (Basic/Scale/Enterprise) recorded as metadata; OSS reports a resource footprint with no dollars. - Sensitive-field redaction on by default (SQL text, auth params, IPs, row-policy filters). - Unit tests for variants, connector, extract, configurator, and CLI; docs page and profiler index entry.
|
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2574 +/- ##
==========================================
+ Coverage 71.19% 71.52% +0.32%
==========================================
Files 111 114 +3
Lines 10071 10355 +284
Branches 1113 1153 +40
==========================================
+ Hits 7170 7406 +236
- Misses 2687 2717 +30
- Partials 214 232 +18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
✅ 179/179 passed, 2 skipped, 2h35m9s total Running from acceptance #5408 |
Addresses the Isaac Review finding: only CostsCollector wrapped
system.query_log in clusterAllReplicas() on ClickHouse Cloud, so
Workload/Features/Dependencies/Utilization/Security queried per-node log
tables directly and under-reported on multi-replica Cloud services (ClickHouse
docs: query_log is local to each node; a complete view needs clusterAllReplicas).
- Add BaseCollector.source(table): wraps per-node append-only log tables
(query_log, session_log, query_views_log, asynchronous_insert_log) in
clusterAllReplicas('default', ...) on Cloud; replicated metadata stays direct.
- Detect is_cloud once in the extract and share via config; enable
skip_unavailable_shards once as a session setting (drops per-query SETTINGS).
- Use self.source(...) across all six collectors; also fixes the latent
asynchronous_insert_log gap in costs. Remove the duplicate CostsCollector._detect_cloud.
- Reset the output DuckDB at the start of each run so re-running a different
variant into the same folder replaces variant-shaped tables (e.g.
costs_pricing_config) instead of TRUNCATE-into-stale-schema.
- Tests: source() wrapping, all collectors cluster-read on Cloud, same-folder
variant re-run reset. Verified live on OSS + Cloud.
dey-abhishek
changed the title
The costs collector reported only the profiled service's billed cost, which understates total cost of ownership when an org has org-level charges (backups, ClickPipes, shared) or multiple services not attributed to a single serviceId. - summarize_usage_cost now surfaces org_total_usd from the usageCost grandTotalCHC (org-wide grand total across all services + org-level charges). - actual_billed_cost now reports service_total_usd (profiled service), org_total_usd (TCO), and total_usd/monthly_total_usd as the TCO headline (org total when available, else the service total). - Unit tests for the org-total TCO logic and the no-grand-total fallback.
dey-abhishek
enabled auto-merge
July 19, 2026 19:57
Blocking review fixes (PR #2574): - Share host-derived TLS/port normalization (normalize_secure_and_port) across the extraction ClickHouseConnection and the probe ClickHouseConnector so a Cloud host is never plaintext-by-default and a stray secure:"false" can't downgrade it; parse_bool fixes bool("false"). - Redact previously-missed sensitive fields: dictionaries.source, mutations.command/latest_fail_reason, columns.default_expression. - Rename the extrapolated monthly_total_usd to monthly_total_usd_projected so it can't be mistaken for an actual bill (actual-billed-only contract). - Honor the cloud_api.tier override (was silently dropped; only top-level tier was read). Non-blocking: - safe_query classifies missing-object errors (WARN) vs real permission/SQL/connection errors (surfaced as ERROR). - Coerce days_back in execute() instead of aborting on a bad value. - _detect_region uses the suffix host-match helper, not a substring. - Docs: soften the "every table always created" claim. Fix configure-database-profiler tier prompt looping on Enter: an empty question default re-prompts in blueprint, so gate the tier override behind a yes/no confirm + choice. Docs: add a Credentials File Format section (sanitized OSS/Cloud examples). Tests: add coverage for extraction-connection secure defaults, redaction of the new fields, no extrapolated dollar field, tier override winning over the API, safe_query classification, days_back coercion, and the tier-override confirm flow (incl. auto-detect default). Co-authored-by: Isaac
Contributor
|
@dey-abhishek why are we using a python script instead of SQL? |
# Conflicts: # src/databricks/labs/lakebridge/connections/database_manager.py # tests/unit/assessment/test_assessment.py # tests/unit/connections/test_database_manager.py # uv.lock
Contributor
Author
|
Good question. The short answer is that the ClickHouse collectors aren't a fixed set of queries — they make decisions while they run, so a plain A few concrete reasons:
It also matches what we already do — Synapse and BigQuery are Python extract scripts for the same reasons. The |
- Redact sensitive fields nested inside struct/map columns (redact_structure), not just top-level keys, on the query-result row path. - Harden ClickHouseCloudAPI._get: raise CloudAPIError on non-JSON / non-dict responses. - Normalize host case in discover_service so a case-only mismatch still resolves the service. - Strengthen the schema-coverage test to drive collect() under both variants (catches dynamic result-set keys a regex scan misses). - _detect_cloud re-raises real connection/permission errors instead of silently treating them as OSS; only genuine missing-object errors degrade to OSS. Verified end-to-end against live OSS and Cloud ClickHouse instances. Co-authored-by: Isaac
Docstring-only edit; no behavior change. Co-authored-by: Isaac
The merge in 17e2d03 regenerated uv.lock with local dev-proxy (pypi-proxy.dev.databricks.com) registry and package URLs, which CI cannot reach. Rewrite them back to public pypi.org / files.pythonhosted.org, matching the scrub in `make lock-dependencies`. No dependency versions change. Co-authored-by: Isaac
Replaces the cross-module protected-access call (which needed a `# pylint: disable=protected-access` the CI no-cheat check forbids) by moving _is_missing_object_error to a module-level `is_missing_object_error` in base.py. No behavior change. Co-authored-by: Isaac
- Rename _detect_cloud -> detect_cloud (public) so the unit tests calling it no longer trigger protected-access (W0212). - Absorb urlopen's timeout kwarg via **_kwargs in the cloud_api test fake (unused-argument, W0613). - Correct a stale CostsCollector._detect_cloud reference in a test docstring. No behavior change. Co-authored-by: Isaac
Replace the monolithic Python extractor (ch_metadata_extract.py + 7 collector classes + connection/redaction helpers, ~2,150 lines) with paired ddl+sql profiler steps, matching the mssql/oracle/teradata profilers. Each of the ~64 result sets is now a typed DuckDB DDL plus a system.* SQL query. - Shared replicated-metadata SQL lives in the clickhouse/ root; per-node log-table reads (query_log, session_log, query_views_log, asynchronous_insert_log) are split into oss/ (direct) and cloud/ (clusterAllReplicas) variants, like mssql single_db/ multi_db. Log-table steps are optional: true so a build without them degrades to ABSENT instead of failing the run. - Redaction is always-on via '[REDACTED]' SQL literals (sensitive columns never leave the source); array columns are JSON-encoded with toJSONString(). The query-history window is a fixed 30 days. The now-dead days_back/redact configurator knobs are removed. - The Cloud billing enrichment is the one remaining Python step (cost_enrich.py), run optionally in the cloud/ pipeline only, since the usageCost figures live behind the ClickHouse Cloud REST API. OSS emits a static pricing_config row via SQL. - Wrap ClickHouseConnector.fetch driver errors in ConnectionError so the pipeline can classify a missing/disabled system table as ABSENT (mirrors the other connectors). Verified end-to-end against a local OSS container and a live ClickHouse Cloud service (both variants: all steps COMPLETE, session_log ABSENT on OSS, real billed cost pulled on Cloud). Updates configurator, tests, and docs accordingly. Co-authored-by: Isaac
Contributor
Author
|
Hi @m-abulazm - The profiler's collector feature has been now rewritten into SQL/DDL steps, as requested |
m-abulazm
requested changes
Aug 4, 2026
Comment on lines
+19
to
+21
| ### 1. Download | ||
| - No ODBC driver is required. The ClickHouse profiler connects over the HTTP interface using the | ||
| [`clickhouse-connect`](https://clickhouse.com/docs/integrations/python) client, which is installed with Lakebridge. |
Contributor
There was a problem hiding this comment.
please remove this part. why even mention something if it is not needed nor in-scope
Comment on lines
+509
to
+515
| Connection keys (``host``/``port``/``user``/``password``/``secure``) are stored flat so the | ||
| ``ClickHouseConnector`` and the OSS-vs-Cloud variant probe can read them directly. The profiler | ||
| itself is SQL/DDL: the query-history window is a fixed 30 days and sensitive fields (SQL text, auth | ||
| params, host IPs, row-policy filters) are always redacted to ``[REDACTED]`` in the SQL, so there | ||
| are no ``days_back`` / ``redact`` knobs. An optional ``cloud_api`` block enables pulling the actual | ||
| billed cost + real sizing + plan tier from the ClickHouse Cloud API (Cloud only). | ||
| """ |
Contributor
There was a problem hiding this comment.
Suggested change
| Connection keys (``host``/``port``/``user``/``password``/``secure``) are stored flat so the | |
| ``ClickHouseConnector`` and the OSS-vs-Cloud variant probe can read them directly. The profiler | |
| itself is SQL/DDL: the query-history window is a fixed 30 days and sensitive fields (SQL text, auth | |
| params, host IPs, row-policy filters) are always redacted to ``[REDACTED]`` in the SQL, so there | |
| are no ``days_back`` / ``redact`` knobs. An optional ``cloud_api`` block enables pulling the actual | |
| billed cost + real sizing + plan tier from the ClickHouse Cloud API (Cloud only). | |
| """ | |
| """ |
| } | ||
| _save_to_disk(credential, cred_file) | ||
|
|
||
| logger.info(f"Credential template created for {source}.") |
Contributor
There was a problem hiding this comment.
Suggested change
| logger.info(f"Credential template created for {source}.") |
Comment on lines
+299
to
+301
| Not SQLAlchemy-based (like Redshift), so it subclasses the ABC directly. Works against both | ||
| ClickHouse Cloud (``secure=True``, port 8443) and self-managed / OSS (port 8123). Only read-only | ||
| ``system.*`` metadata queries are ever run through it. |
Contributor
There was a problem hiding this comment.
Suggested change
| Not SQLAlchemy-based (like Redshift), so it subclasses the ABC directly. Works against both | |
| ClickHouse Cloud (``secure=True``, port 8443) and self-managed / OSS (port 8123). Only read-only | |
| ``system.*`` metadata queries are ever run through it. |
Comment on lines
+20
to
+36
| def parse_bool(value: object, default: bool = False) -> bool: | ||
| """Parse a credential value that may be a real bool or a string like ``"false"``. | ||
|
|
||
| ``bool("false")`` is ``True`` in Python, so a hand-written credentials file that stores | ||
| ``secure: "false"`` must be parsed on the string content, not coerced with ``bool()``. A real | ||
| bool passes through; a string is matched case-insensitively against the truthy token set; | ||
| anything else falls back to ``default``. | ||
| """ | ||
| if isinstance(value, bool): | ||
| return value | ||
| if value is None: | ||
| return default | ||
| if isinstance(value, str): | ||
| return value.strip().lower() in _TRUTHY | ||
| if isinstance(value, (int, float)): | ||
| return bool(value) | ||
| return default |
Contributor
There was a problem hiding this comment.
our confirm prompts always return True or False literals. this extra parsing is unnecessary
| return default | ||
|
|
||
|
|
||
| def normalize_secure_and_port(config: dict) -> tuple[bool, int]: |
Contributor
There was a problem hiding this comment.
can we embed this inside the connector
Comment on lines
+310
to
+314
| # The configurator always writes `secure` explicitly; this fallback is for a minimal | ||
| # hand-written credentials file. normalize_secure_and_port forces TLS/8443 for managed | ||
| # ClickHouse Cloud hosts (which only accept TLS) and defaults to plaintext/8123 for | ||
| # self-managed / OSS — never insecure-by-default for Cloud. Shared with the extraction | ||
| # ClickHouseConnection so both paths behave identically. |
Contributor
There was a problem hiding this comment.
Suggested change
| # The configurator always writes `secure` explicitly; this fallback is for a minimal | |
| # hand-written credentials file. normalize_secure_and_port forces TLS/8443 for managed | |
| # ClickHouse Cloud hosts (which only accept TLS) and defaults to plaintext/8123 for | |
| # self-managed / OSS — never insecure-by-default for Cloud. Shared with the extraction | |
| # ClickHouseConnection so both paths behave identically. |
Comment on lines
+325
to
+327
| # Wrap driver errors as ConnectionError so the pipeline can classify them: an optional step | ||
| # that hits a missing/disabled system table (e.g. system.session_log on an OSS build) degrades | ||
| # to ABSENT instead of aborting the whole run. Mirrors the other connectors (e.g. Redshift). |
Contributor
There was a problem hiding this comment.
Suggested change
| # Wrap driver errors as ConnectionError so the pipeline can classify them: an optional step | |
| # that hits a missing/disabled system table (e.g. system.session_log on an OSS build) degrades | |
| # to ABSENT instead of aborting the whole run. Mirrors the other connectors (e.g. Redshift). |
| } | ||
|
|
||
|
|
||
| def execute(credential_manager: CredentialManager, db_path: str) -> dict[str, Any]: |
Contributor
There was a problem hiding this comment.
Suggested change
| def execute(credential_manager: CredentialManager, db_path: str) -> dict[str, Any]: | |
| def execute(credential_manager: CredentialManager, db_path: str, client: ClickHouseCloudAPI) -> dict[str, Any]: |
please pass client to substitute the dependency in the tests
# Conflicts: # tests/unit/assessment/test_assessment.py # tests/unit/test_cli_other.py
- docs: drop out-of-scope Download/ODBC note, renumber prerequisites - configure_assessment: trim ClickHouse configurator docstring, drop the redundant trailing log line - database_manager: inline secure/port normalization into ClickHouseConnector and trim verbose comments; drop the shared normalize_secure_and_port helper - clickhouse/__init__: remove now-unused parse_bool (confirm prompts return real bools) and normalize_secure_and_port - cost_enrich: inject ClickHouseCloudAPI client into execute() so tests can substitute the dependency; add execute() coverage - _constants: register clickhouse as an AUTO variant source
# Conflicts: # pyproject.toml
New costs_read_locality metric attributes query reads to the local filesystem cache vs the object-storage source (S3) using query_log ProfileEvents (CachedReadBufferReadFromCacheBytes vs CachedReadBufferReadFromSourceBytes, plus ReadBufferFromS3Bytes and S3ReadRequestsCount), reporting a cache_hit_pct. Wired into the oss and cloud pipeline configs (ddl + sql). Docs updated with the metric, the Read Locality section, and the bare-minimum Cloud API-key roles (Basic Service API Reader + Billing).
… steps - cloud_api: fold unnamed CHC buckets (ClickPipes, dictionary, etc.) into `other` so the per-service billed total covers the whole metrics map - database_manager: correct a cloud host's plaintext default port to the secure port so the forced-TLS override connects to the right place - cost_enrich: report region alone for legacy hosts instead of guessing an AWS provider; cap the usage window at 30 inclusive dates (today - 29d) - pipeline_config (cloud + oss): mark security_roles and security_settings_profiles steps optional
Contributor
Author
# Conflicts: # src/databricks/labs/lakebridge/connections/database_manager.py # tests/unit/connections/test_database_manager.py
ClickHouse returns count/byte/row/memory counters (and its unlimited sentinel) as UInt64, whose upper half exceeds signed BIGINT range. DuckDB raises a conversion error on insert that the profiler's optional:true net does not catch, crashing the whole run instead of skipping one table. Match the DuckDB column type to the ClickHouse source type: UInt64-sourced metrics -> UBIGINT (as normalized_query_hash already does); leave UInt8 flags and signed Int32/Int64 sources (system.metrics.value, exception_code, mutations.parts_to_do, metric_log CurrentMetric_* peaks) as BIGINT.
Contributor
Author
|
Added UBIGINT changes as per code review comment by @Rubjit |
- Report org-wide TCO (grandTotalCHC) even when the profiled service has no billed records yet: gate the billed-cost block on record_count OR an org grand total, via _has_billed_cost(). Previously record_count==0 silently dropped the org total. - Use a distinct note when metadata is captured but no billed cost is available, instead of _ENRICHED_NOTE claiming dollar figures that are absent. - Degrade gracefully on a reachable-but-malformed API response: broaden execute()'s except around _fetch_cloud_metadata to structural errors (KeyError/IndexError/TypeError) that discover_service can raise, so the optional step writes a no-enrichment row instead of aborting the run. Add regression tests for all three.
Cloud profiler SQL reads replicated system tables via
clusterAllReplicas('default', ...). ClickHouse Cloud always names its
cluster 'default', but a self-managed deployment profiled as cloud may use
a different name, making those (optional) steps fail with
CLUSTER_DOESNT_EXIST and come back empty.
ClickHouseConnector now honors an optional 'cluster' config value: when set
to something other than 'default', it rewrites the clusterAllReplicas cluster
in outgoing queries. Default behavior is unchanged (Cloud's 'default' stays
authoritative), and the name is validated against the identifier charset
before being spliced into SQL. Scoped entirely to ClickHouseConnector.
Verified e2e on an OSS box with a 'main_cluster' (no 'default') forced to the
cloud variant: without the override 33 cluster steps failed to ABSENT; with
cluster: main_cluster all resolved (153 COMPLETE, 0 CLUSTER_DOESNT_EXIST).
Explain that the Cloud variant reads via clusterAllReplicas('default', ...),
that ClickHouse Cloud always uses 'default' (no config needed), and that a
self-managed deployment profiled as Cloud with a differently-named cluster
sets 'cluster' in the credentials file to avoid CLUSTER_DOESNT_EXIST.
summarize_usage_cost bucketed metrics with case-sensitive exact keys and a 'DataTransferCHC' substring, so a re-cased/renamed metric key would fall into 'other' instead of its proper bucket. Classify each metric in a single case-insensitive pass; 'other' stays the catch-all so the total always covers the whole metrics map. Breakdown-only hardening: the total was already correct.
Avoids pylint W0212 (protected-access) by matching a distinctive phrase in the note text, mirroring the existing no-creds test's style.
Tighten the explanatory comments added for the review fixes (cluster override, billed-cost gating, malformed-API handling, case-insensitive bucketing) while keeping the essential rationale.
# Conflicts: # src/databricks/labs/lakebridge/assessments/_constants.py # uv.lock
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
What does this PR do?
Adds ClickHouse as a first-class citizen profiler source on Lakebridge, covering both ClickHouse Cloud and self-managed / OSS. It conforms to the existing Lakebridge profiler contract (registration, credential configurator, connector, pipeline config, DuckDB output), so it behaves consistently with the other sources (mssql, bigquery, redshift, synapse, oracle, teradata, snowflake).
The analytics portion of the standalone tool is out of scope; only the read-only
system.*metadata profiling is ported.Architecture: SQL/DDL steps (not a Python extractor)
The profiler is built from paired
ddl+sqlprofiler steps, matching the mssql/oracle/teradata profilers — there is no monolithic Python extractor or collector framework. Each of the ~64 result sets is a typed DuckDBCREATE TABLE(theddlstep) plus a read-onlysystem.*query (thesqlstep). SQL is easier to maintain and debug than a custom Python script, and it keeps ClickHouse consistent with every other SQL-based source.Relevant implementation details
clickhouseadded toPROFILER_SOURCE_SYSTEMwith anAUTOvariant. Theossvscloudvariant is auto-detected at run time (no user selection), mirroring the mssql pattern: a*.clickhouse.cloudhost or thecloud_modeserver setting decides.clickhouse/root and are shared by both variants. The per-node append-only log tables (query_log,session_log,query_views_log,asynchronous_insert_log) are split intooss/(read directly) andcloud/(wrapped inclusterAllReplicas('default', ...)so all replicas are covered) — the same shared-root + variant-dir layout as mssqlsingle_db/multi_db. Log-table steps areoptional: true, so a build without a given table degrades toABSENTinstead of failing the run.ClickHouseConnectorover theclickhouse-connectHTTP client (native-client style, likeRedshiftConnector, not SQLAlchemy).secure/port defaults are host-derived (TLS + 8443 for Cloud hosts, plaintext + 8123 otherwise) so it is never insecure-by-default for Cloud. Driver errors are wrapped inConnectionErrorso the pipeline can classify a missing/disabledsystem.*table asABSENT(mirrors the other connectors).ConfigureClickHouseAssessmentprompts host/port/user/password/secure and an optional Cloud API block (key_id/key_secret, optional org/service id, optional tier override). There are nodays_back/redactknobs: the query-history window is a fixed 30 days and redaction is always on (see below).'[REDACTED]'literal directly in thesqlstep, so the real value never leaves the source. Array/tuple columns are JSON-encoded withtoJSONString(). Aggregate metrics, counts, and grant structure are always preserved.ddlstep creates the typed table before thesqlstep populates it, the output schema is stable across runs and deployments even when a result set is empty (consistent with the mssql*_ddl.sqlapproach).usageCostAPI. This is the one remaining Python step (cloud/cost_enrich.py), run optionally in the Cloud pipeline only, since the billing figures live behind the Cloud REST API and SQL cannot reach them. It writes provider/region, real compute sizing, plan tier (recorded as metadata, no pricing math), and the billed cost intocosts_pricing_config. OSS emits a staticcosts_pricing_configrow via SQL (no dollar figures) and always reports a resource footprint.Caveats/things to watch out for when reviewing:
clickhouse-connect~=1.4.2(+ transitivelz4,zstandard).cost_enrich.pyis the sole Python step; ifcloud_apicredentials are absent or the API is unreachable it writes a "no enrichment" row and the step still succeeds (it isoptional: true).INTERVAL 30 DAYin the log-table SQL.COMPLETE(withsession_logcorrectlyABSENTon OSS), 77 tables per run, redaction, Cloud actual-cost + tier detection, OSS footprint-only.Linked issues
Resolves #..
Functionality
docs/.../assessment/profiler/clickhouse.mdx+ profiler index entry)databricks labs lakebridge configure-database-profiler/execute-database-profiler(newclickhousesource)Tests