Skip to content

Commit d56a556

Browse files
[SDBM-2746] Collect standard system-table metrics per node in single endpoint mode (DataDog#24266)
* Collect standard system-table metrics per node in single endpoint mode In single endpoint mode the agent reaches a multi-node ClickHouse cluster through one load balancer, but the standard metric queries read per-node system tables directly (bare system.events/metrics/errors). Consecutive scrapes land on different nodes, so cumulative counters appear to jump up and down and the backend reports phantom monotonic_count spikes such as the false clickhouse.query.failed.count in SDBM-2746. Route the bulk-match metric queries and system.errors through clusterAllReplicas() and tag each row with hostName() when single_endpoint_mode is enabled, giving each node its own metric series (counters increment monotonically per node; gauges stop flapping). Direct connections are unchanged. system.parts/replicas/dictionaries use GROUP BY and are left for a follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Add changelog entry Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(clickhouse): simplify make_cluster_aware with regex subn Replace the manual search + start()/end() slicing with a single SYSTEM_TABLE_FROM_CLAUSE.subn() pass (count=1). The regex now consumes the whitespace before FROM so the per-node projection splices in without a gap before the comma, keeping the rewritten query byte-for-byte identical. Output and behavior are unchanged; all unit tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style(clickhouse): fix ruff format of assert message in test_unit ruff 0.11.10 reformats the parenthesized assert condition back to the trailing-message form. Apply it so `ddev test --fmt` / CI lint passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(clickhouse): trim make_cluster_aware docstring to a one-liner Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Replace runtime regex with static cluster-aware query variants Instead of rewriting system-table metric queries with a regex at runtime, define static cluster-aware query variants and select them in get_queries() based on single_endpoint_mode. - Add cluster_aware_query() helper in utils.py that reuses the base query's columns by reference (no duplication) and appends the clickhouse_node tag. - Define legacy variants in queries.py and advanced variants in advanced_queries (JSON-backed via load_match_query + SystemErrorsClusterAware). - Remove import re, SYSTEM_TABLE_FROM_CLAUSE, and make_cluster_aware(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Drop ticket number from test docstring Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Simplify cluster_aware_query to derive SELECT/table from base query cluster_aware_query now takes only the base query dict and derives the SELECT list and table from base['query'], so call sites no longer restate column lists or table names. load_match_query reverts to its original signature; __getattr__ wraps the loaded query for ClusterAware names. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Build cluster-aware variants inline in get_queries Drop the static *ClusterAware definitions and the __getattr__ suffix handling; get_queries wraps each base query via cluster_aware_query() when single_endpoint_mode is on. queries.py and advanced_queries revert to base; the change now touches only utils.py, clickhouse.py, and the tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 283cdf2 commit d56a556

4 files changed

Lines changed: 112 additions & 10 deletions

File tree

clickhouse/changelog.d/24266.fixed

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Collect standard system table metrics per node via clusterAllReplicas in single endpoint mode so per-node counters no longer produce phantom failures (e.g. clickhouse.query.failed.count).

clickhouse/datadog_checks/clickhouse/clickhouse.py

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323
from .statement_samples import ClickhouseStatementSamples
2424
from .statements import ClickhouseStatementMetrics
2525
from .table_metrics import ClickhouseTableMetrics
26-
from .utils import ErrorSanitizer
26+
from .utils import ErrorSanitizer, cluster_aware_query
2727

2828
try:
2929
import datadog_agent
@@ -279,14 +279,19 @@ def check(self, _):
279279

280280
def get_queries(self) -> list[dict]:
281281
query_list = []
282+
single = self._config.single_endpoint_mode
283+
284+
def pick(query: dict) -> dict:
285+
"""In single endpoint mode, read all replicas and tag each row per node."""
286+
return cluster_aware_query(query) if single else query
282287

283288
if self._config.use_legacy_queries:
284289
query_list.extend(
285290
[
286-
queries.SystemMetrics,
287-
queries.SystemEventsToDeprecate,
288-
queries.SystemEvents,
289-
queries.SystemAsynchronousMetrics,
291+
pick(queries.SystemMetrics),
292+
pick(queries.SystemEventsToDeprecate),
293+
pick(queries.SystemEvents),
294+
pick(queries.SystemAsynchronousMetrics),
290295
queries.SystemParts,
291296
queries.SystemReplicas,
292297
queries.SystemDictionaries,
@@ -296,13 +301,13 @@ def get_queries(self) -> list[dict]:
296301
if self._config.use_advanced_queries:
297302
query_list.extend(
298303
[
299-
advanced_queries.SystemMetrics,
300-
advanced_queries.SystemEvents,
301-
advanced_queries.SystemAsynchronousMetrics,
304+
pick(advanced_queries.SystemMetrics),
305+
pick(advanced_queries.SystemEvents),
306+
pick(advanced_queries.SystemAsynchronousMetrics),
302307
]
303308
)
304309
if self.version_ge('21.3'):
305-
query_list.append(advanced_queries.SystemErrors)
310+
query_list.append(pick(advanced_queries.SystemErrors))
306311

307312
return query_list
308313

clickhouse/datadog_checks/clickhouse/utils.py

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,5 +27,27 @@ def compact_query(query):
2727
return re.sub(r'\n\s+', ' ', query.strip())
2828

2929

30+
# Tag added to per-node metrics when collecting from all replicas in single endpoint mode.
31+
CLUSTER_NODE_TAG = 'clickhouse_node'
32+
33+
34+
def cluster_aware_query(base: dict) -> dict:
35+
"""Build a cluster-aware variant that reads all replicas and tags each row per node.
36+
37+
Derives the SELECT list and table from the base query, whose shape is always
38+
``SELECT <cols> FROM system.<table>[ <trailing clause>]``.
39+
"""
40+
select, _, tail = base['query'].partition(' FROM system.')
41+
table, sep, trailing = tail.partition(' ')
42+
return {
43+
'name': base['name'],
44+
'query': (
45+
f"{select}, hostName() AS {CLUSTER_NODE_TAG} "
46+
f"FROM clusterAllReplicas('default', system.{table}){sep}{trailing}"
47+
),
48+
'columns': [*base['columns'], {'name': CLUSTER_NODE_TAG, 'type': 'tag'}],
49+
}
50+
51+
3052
def parse_version(version: str) -> list[int]:
3153
return [int(v) for v in version.split('.')]

clickhouse/tests/test_unit.py

Lines changed: 75 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@
66
from clickhouse_connect.driver.exceptions import Error, OperationalError
77

88
from datadog_checks.base import ConfigurationError
9-
from datadog_checks.clickhouse import ClickhouseCheck, advanced_queries
9+
from datadog_checks.clickhouse import ClickhouseCheck, advanced_queries, queries
10+
from datadog_checks.clickhouse.utils import cluster_aware_query
1011

1112
from .utils import ensure_csv_safe, parse_described_metrics, raise_error
1213

@@ -381,3 +382,76 @@ def test_database_hostname_ignores_reported_hostname_override(reported_hostname,
381382
# reported_hostname honors the override when configured, otherwise the resolved host
382383
assert check.reported_hostname == expected_reported_hostname
383384
mock_resolve.assert_called_with(BASE_INSTANCE['server'])
385+
386+
387+
def test_cluster_aware_query_bulk_match_query():
388+
"""The cluster-aware variant reads all replicas and tags system.events per node."""
389+
variant = cluster_aware_query(advanced_queries.SystemEvents)
390+
391+
assert variant['query'] == (
392+
"SELECT value, event, hostName() AS clickhouse_node FROM clusterAllReplicas('default', system.events)"
393+
)
394+
assert variant['columns'][-1] == {'name': 'clickhouse_node', 'type': 'tag'}
395+
# The base query dict must not be mutated by building the variant.
396+
assert advanced_queries.SystemEvents['query'] == 'SELECT value, event FROM system.events'
397+
assert all(column['name'] != 'clickhouse_node' for column in advanced_queries.SystemEvents['columns'])
398+
399+
400+
def test_cluster_aware_query_preserves_where_clause():
401+
"""system.errors carries a WHERE clause that must survive in the cluster-aware variant."""
402+
variant = cluster_aware_query(advanced_queries.SystemErrors)
403+
404+
assert variant['query'] == (
405+
"SELECT value, name, code, remote, hostName() AS clickhouse_node "
406+
"FROM clusterAllReplicas('default', system.errors) WHERE value > 0"
407+
)
408+
assert variant['columns'][-1] == {'name': 'clickhouse_node', 'type': 'tag'}
409+
410+
411+
def test_cluster_aware_query_legacy_query():
412+
"""The helper builds a cluster-aware variant for a legacy query too."""
413+
variant = cluster_aware_query(queries.SystemMetrics)
414+
415+
assert variant['query'] == (
416+
"SELECT value, metric, hostName() AS clickhouse_node FROM clusterAllReplicas('default', system.metrics)"
417+
)
418+
assert variant['columns'][-1] == {'name': 'clickhouse_node', 'type': 'tag'}
419+
assert queries.SystemMetrics['query'] == 'SELECT value, metric FROM system.metrics'
420+
421+
422+
@pytest.mark.parametrize('use_advanced_queries', [True, False])
423+
def test_get_queries_tags_system_tables_per_node_in_single_endpoint_mode(instance, use_advanced_queries):
424+
instance = {
425+
**instance,
426+
'single_endpoint_mode': True,
427+
'use_advanced_queries': use_advanced_queries,
428+
'use_legacy_queries': not use_advanced_queries,
429+
}
430+
check = ClickhouseCheck('clickhouse', {}, [instance])
431+
check._server_version = '24.8'
432+
433+
cluster_aware = [q for q in check.get_queries() if 'clusterAllReplicas' in q['query']]
434+
435+
# system.events, system.metrics, system.asynchronous_metrics (+ system.errors / events-to-deprecate)
436+
assert cluster_aware
437+
for query in cluster_aware:
438+
assert 'hostName() AS clickhouse_node' in query['query']
439+
assert query['columns'][-1] == {'name': 'clickhouse_node', 'type': 'tag'}
440+
441+
# system.parts/replicas/dictionaries use GROUP BY and are intentionally left untouched here.
442+
if not use_advanced_queries:
443+
assert any(q is queries.SystemParts for q in check.get_queries())
444+
445+
446+
@pytest.mark.parametrize('use_advanced_queries', [True, False])
447+
def test_get_queries_uses_base_queries_for_direct_connection(instance, use_advanced_queries):
448+
instance = {
449+
**instance,
450+
'single_endpoint_mode': False,
451+
'use_advanced_queries': use_advanced_queries,
452+
'use_legacy_queries': not use_advanced_queries,
453+
}
454+
check = ClickhouseCheck('clickhouse', {}, [instance])
455+
check._server_version = '24.8'
456+
457+
assert all('clusterAllReplicas' not in q['query'] for q in check.get_queries())

0 commit comments

Comments
 (0)