feat: verify ClickHouse events table and query customer event stats - #8116
feat: verify ClickHouse events table and query customer event stats#8116Zaimwa9 wants to merge 3 commits into
Conversation
…gsmith_exp database
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
@themis-blindfold review |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
api
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8116 +/- ##
=======================================
Coverage 98.69% 98.70%
=======================================
Files 1525 1525
Lines 60607 60661 +54
=======================================
+ Hits 59819 59873 +54
Misses 788 788 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![themis-blindfold[bot]](https://avatars.githubusercontent.com/in/4278417?s=60&v=4){kind=small}
⚖️ Themis judgement: 🟠 Fix before mergeVerifying the One
🟠 Majors
🧹 Nits
📝 Walkthrough
🧪 How to verify
Product take: Makes BYO-ClickHouse onboarding self-diagnosing — a missing Big picture: 🧭 Assumptions & unverified claims
Verifying the table exists before trusting it is good manners; now the frontend just needs to agree on which database to look in. · reviewed at 361572d 1 finding(s) anchored outside the diff (not posted inline)
This GET test now runs the live-connection path unmocked. |
github-actions
Bot
added
docs
|
Review was cancelled before completing (worker timeout or shutdown). Mention @themis-blindfold with |
Docker builds report
|
github-actions
Bot
added
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: db174986-e9ac-462c-8943-b581082c35b1
📒 Files selected for processing (8)
api/app/settings/common.pyapi/app/settings/test.pyapi/experimentation/services.pyapi/experimentation/types.pyapi/experimentation/views.pyapi/tests/unit/experimentation/test_services.pyapi/tests/unit/experimentation/test_views.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.md
| def _connect_customer_clickhouse(connection: WarehouseConnection) -> Client: | ||
| """Build a client for the customer's ClickHouse instance from stored | ||
| connection details, re-checking the host against internal address ranges | ||
| first: DNS may resolve differently than it did at validation time, and | ||
| rows may predate host validation.""" | ||
| config = typing.cast(ClickHouseConfig, connection.config or {}) | ||
| credentials = typing.cast(ClickHouseCredentials, connection.credentials or {}) | ||
| if is_internal_address(config["host"], include_shared=True): | ||
| raise InternalAddressError(config["host"]) | ||
| return Client( | ||
| config["host"], | ||
| port=config["port"], | ||
| user=config["username"], | ||
| password=credentials["password"], | ||
| database=config["database"], | ||
| secure=config["secure"], | ||
| connect_timeout=CLICKHOUSE_CONNECT_TIMEOUT_SECONDS, | ||
| send_receive_timeout=CLICKHOUSE_VERIFY_TIMEOUT_SECONDS, | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect is_internal_address to see whether it resolves DNS and whether resolution is reused for the connection.
rg -n -B2 -A25 'def is_internal_address' api/experimentation/services.pyRepository: Flagsmith/flagsmith
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find files mentioning is_internal_address/ClickHouse/client:"
rg -n "is_internal_address|ClickHouse.*Config|def _connect_customer_clickhouse|Client\(" . || true
echo
echo "List likely files under api/experimentation:"
fd -a . api/experimentation 2>/dev/null | sed 's#^\./##' | head -100Repository: Flagsmith/flagsmith
Length of output: 18599
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect core/network.py:"
cat -n api/core/network.py
echo
echo "Inspect experimentation/services.py near Client helper and customer connect code:"
sed -n '1,180p' api/experimentation/services.py | cat -n
echo "---"
sed -n '780,850p' api/experimentation/services.py | cat -n
echo
echo "Search for dnslib/resolve/Client import in experimentation services:"
rg -n "dns|resolve|Client|Warehouse|warehouse|connect_timeout|secure" api/experimentation/services.py api/core/network.py api/experimentation/warehouse_validation.pyRepository: Flagsmith/flagsmith
Length of output: 18526
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from pathlib import Path
services_path = Path("api/experimentation/services.py")
network_path = Path("api/core/network.py")
services_tree = ast.parse(services_path.read_text())
network_tree = ast.parse(network_path.read_text())
for node in services_tree.body:
if isinstance(node, ast.FunctionDef) and node.name == "_connect_customer_clickhouse":
print("services._connect_customer_clickhouse:")
print(ast.unparse(node))
print("--- source line numbers:", node.lineno, "-", node.end_lineno)
print()
for node in network_tree.body:
if isinstance(node, ast.FunctionDef) and node.name == "is_internal_address":
print("core.network.is_internal_address:")
print(ast.unparse(node))
print("--- source line numbers:", node.lineno, "-", node.end_lineno)
print
print("Imports at api/experimentation/services.py line 1-12:")
for node in services_tree.body[:15]:
if isinstance(node, ast.ImportFrom):
line = ast.get_source_segment(services_path.read_text(), node)
print(f"{node.lineno}: {line}")
PY
echo
echo "Relevant static call-chain:"
rg -n "def _get_customer_warehouse_event_stats_cached|def .*event_stats.*customer|WarehouseConnection|warehouse_type" api/experimentation/services.py | sed -n '1,120p'Repository: Flagsmith/flagsmith
Length of output: 3779
Resolve and pin the ClickHouse host address before connecting.
is_internal_address() is only one independent host-name lookup; Client() resolves the hostname again during connection. A customer-controlled, low-TTL hostname can resolve to a public address through the first check and then to a private/internal/metadata address when the client connects, bypassing this guard entirely. Resolve the hostname once, reject internal addresses among the resolved IPs, and connect using the pinned IP while preserving the original hostname for TLS SNI/hostname verification when secure=True.
| return Client( | ||
| config["host"], | ||
| port=config["port"], | ||
| user=config["username"], | ||
| password=credentials["password"], | ||
| database=config["database"], | ||
| secure=config["secure"], | ||
| connect_timeout=CLICKHOUSE_CONNECT_TIMEOUT_SECONDS, | ||
| send_receive_timeout=CLICKHOUSE_VERIFY_TIMEOUT_SECONDS, | ||
| ) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the actual values assigned to these constants.
rg -n 'CLICKHOUSE_VERIFY_TIMEOUT_SECONDS|CLICKHOUSE_QUERY_TIMEOUT_SECONDS|CLICKHOUSE_CONNECT_TIMEOUT_SECONDS' api/experimentation/services.pyRepository: Flagsmith/flagsmith
Length of output: 578
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== outline =="
ast-grep outline api/experimentation/services.py --match '_connect_customer_clickhouse' --view expanded || true
ast-grep outline api/experimentation/services.py --match '_get_customer_warehouse_event_stats_cached' --view expanded || true
ast-grep outline api/experimentation/services.py --match 'verify_clickhouse_connection' --view expanded || true
echo "== relevant lines 800-990 =="
sed -n '800,990p' api/experimentation/services.py | nl -ba -v800
echo "== customer clickhouse usages =="
rg -n '_connect_customer_clickhouse|_query_event_stats|CLICKHOUSE_QUERY_TIMEOUT_SECONDS|CLICKHOUSE_VERIFY_TIMEOUT_SECONDS|verify_clickhouse_connection' api/experimentation/services.pyRepository: Flagsmith/flagsmith
Length of output: 511
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant lines 800-990 =="
python3 - <<'PY'
from pathlib import Path
p = Path('api/experimentation/services.py')
text = p.read_text()
lines = text.splitlines()
for i, line in enumerate(lines[799:990], start=800):
print(f"{i:04d}: {line}")
PY
echo "== customer clickhouse usages =="
rg -n '_connect_customer_clickhouse|_query_event_stats|CLICKHOUSE_QUERY_TIMEOUT_SECONDS|CLICKHOUSE_VERIFY_TIMEOUT_SECONDS|verify_clickhouse_connection' api/experimentation/services.pyRepository: Flagsmith/flagsmith
Length of output: 8967
Use the query timeout for customer event stats.
_connect_customer_clickhouse() shares the 5s CLICKHOUSE_VERIFY_TIMEOUT_SECONDS with verify_clickhouse_connection(), but _get_customer_warehouse_event_stats_cached() uses the same client for full-table count()/uniqExact(event) aggregations. This can make the stats path time out prematurely and return cached unavailable results for 60s; give the stats path the existing 30s CLICKHOUSE_QUERY_TIMEOUT_SECONDS.
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #18771 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #18771 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #18771 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
|
✅ oss · depot-ubuntu-latest-arm-16 — run #18771 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
1 participant
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
eventstable exists; a missing table errors with a "run the setup SQL" detail.flagsmith_exp.How did you test this code?
Unit tests, lint, mypy. Manually: tested a connection without the table (errors with the hint), created it, re-tested, checked stats populate.