Skip to content

refactor(intake): harden ClickHouse access boundary - #942

Merged
BrianNewsom merged 9 commits into
mainfrom
clickhouse-persistence-hardening/brnewsom
Jul 28, 2026
Merged

refactor(intake): harden ClickHouse access boundary#942
BrianNewsom merged 9 commits into
mainfrom
clickhouse-persistence-hardening/brnewsom

Conversation

@BrianNewsom

@BrianNewsom BrianNewsom commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • remove the raw client table-name formatter so runtime identifiers come from the closed table registry
  • remove the obsolete driver-result mapping helper after repository migration
  • update ClickHouse integration utilities to use registered runtime tables
  • enforce exact raw-client import and low-level call boundaries with architecture tests

Stack

Validation

  • Ruff format and lint across Intake: passed
  • targeted ty across repositories and changed cleanup files: passed
  • Intake unit suite: 215 passed
  • complete Intake integration suite on ClickHouse 26.3: 148 passed

Summary by CodeRabbit

  • Refactor

    • Standardized ClickHouse table references across span ingestion, session queries, migrations, and integration test workflows.
    • Removed redundant table and result-conversion helpers.
  • Tests

    • Added architecture checks to ensure low-level ClickHouse operations and client imports remain confined to approved components.
    • Updated integration tests for fully qualified table names and migration verification.

Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
@BrianNewsom
BrianNewsom marked this pull request as ready for review July 28, 2026 16:34
@BrianNewsom
BrianNewsom requested review from a team as code owners July 28, 2026 16:34
Base automatically changed from clickhouse-complete-repository-migration/brnewsom to main July 28, 2026 21:28
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

ClickHouse table qualification now uses shared helpers instead of ClickHouseSpanClient.table(). Unused storage conversion code is removed, integration tests are updated, and architecture tests enforce exact raw-client and low-level-call boundaries.

Changes

ClickHouse boundary cleanup

Layer / File(s) Summary
Remove client and storage helpers
services/intake/src/nmp/intake/spans/clickhouse_client.py, services/intake/src/nmp/intake/spans/storage.py
Migration imports are narrowed, ClickHouseSpanClient.table() is removed, and result_rows() is deleted.
Use shared qualified table helpers
services/intake/tests/integration/spans/*
Fixtures and integration queries use ClickHouseTable, qualified_table, and quoted identifiers for ClickHouse table references.
Enforce low-level access locations
services/intake/tests/test_clickhouse_architecture.py
Architecture tests require exact approved import and low-level ClickHouse call module sets using AST inspection.

Possibly related PRs

Suggested labels: test

Suggested reviewers: shanaiabuggy, mckornfield

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: tightening Intake’s ClickHouse access boundary.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch clickhouse-persistence-hardening/brnewsom

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@services/intake/tests/test_clickhouse_architecture.py`:
- Around line 26-36: Update _imports_raw_client to resolve relative ImportFrom
modules using each source path before matching clickhouse_client, so relative
and absolute imports are normalized consistently. Keep
test_raw_clickhouse_client_imports_are_confined_to_approved_modules unchanged in
enforcing exact equality against _EXPECTED_RAW_CLIENT_IMPORTS.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 0d8f249e-da3f-478f-9714-d50af549e776

📥 Commits

Reviewing files that changed from the base of the PR and between 64cce8d and ab52cc3.

📒 Files selected for processing (7)
  • services/intake/src/nmp/intake/spans/clickhouse_client.py
  • services/intake/src/nmp/intake/spans/storage.py
  • services/intake/tests/integration/spans/conftest.py
  • services/intake/tests/integration/spans/test_clickhouse_bootstrap.py
  • services/intake/tests/integration/spans/test_otlp_ingest_simple.py
  • services/intake/tests/integration/spans/test_sessions_read.py
  • services/intake/tests/test_clickhouse_architecture.py
💤 Files with no reviewable changes (1)
  • services/intake/src/nmp/intake/spans/storage.py

Comment thread services/intake/tests/test_clickhouse_architecture.py
@github-actions

Copy link
Copy Markdown
Contributor
Suite Lines Covered Line Rate Branch Rate
Unit Tests 27559/35314 78.0% 62.5%
Integration Tests 16095/34032 47.3% 19.8%

@BrianNewsom
BrianNewsom added this pull request to the merge queue Jul 28, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 28, 2026
@BrianNewsom
BrianNewsom added this pull request to the merge queue Jul 28, 2026
Merged via the queue into main with commit 4a6d40f Jul 28, 2026
60 checks passed
@BrianNewsom
BrianNewsom deleted the clickhouse-persistence-hardening/brnewsom branch July 28, 2026 23:00
ngoncharenko pushed a commit that referenced this pull request Jul 29, 2026
* test(intake): pin ClickHouse 26.3 LTS

Signed-off-by: Brian Newsom <brnewsom@nvidia.com>

* refactor(intake): add ClickHouse repository boundary

Signed-off-by: Brian Newsom <brnewsom@nvidia.com>

* refactor(intake): migrate evaluation rollup queries

Signed-off-by: Brian Newsom <brnewsom@nvidia.com>

* refactor(intake): migrate evaluation session queries

Signed-off-by: Brian Newsom <brnewsom@nvidia.com>

* refactor(intake): add evaluation read service

Signed-off-by: Brian Newsom <brnewsom@nvidia.com>

* refactor(intake): migrate annotation and evaluator result storage

Signed-off-by: Brian Newsom <brnewsom@nvidia.com>

* refactor(intake): migrate span and trace storage

Signed-off-by: Brian Newsom <brnewsom@nvidia.com>

* refactor(intake): harden ClickHouse access boundary

Signed-off-by: Brian Newsom <brnewsom@nvidia.com>

---------

Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants