mirror of
https://github.com/XRPLF/rippled.git
synced 2026-08-21 06:10:58 +00:00
fix(telemetry): exempt log-datasource queries from naming Rule D
Rule D validated every dashboard label against L1 (*SpanNames.h) and L6
(MetricsRegistry) labels. LogQL labels have a third provenance neither
layer can resolve: they are minted by the collector's regex_parser named
captures (partition, severity) or by an in-query `| regexp` stage
(action, pk, state, mode, phase, jobname, ip, pubkey). Checking them
against L1/L6 reported ten violations for labels correct by
construction.
Make the rule datasource-aware instead of allowlisting a filename. The
dashboard JSON is parsed so each query can be attributed to its
datasource, and queries on a log datasource are skipped. The exemption is
per query, not per file, so a dashboard mixing Prometheus and Loki panels
still has its Prometheus panels validated.
Parsing the JSON also fixed a blind spot: label filters are stored with
backslash-escaped quotes (`label=~\"$v\"`), which the previous raw-text
regex could never match, so only the `sum by (...)` form was ever
checked. With the strings unescaped, 555 queries are now validated where
far fewer were before. That surfaced three legitimate label sources the
rule did not model, each fixed at its source rather than allowlisted:
- deployment_environment / xrpl_network_type: resource attributes the
collector promotes onto metric datapoints. Derived from the config's
resource_metrics_key_attributes, so a new key is picked up
automatically, in both dotted and underscore forms.
- resource.service.instance.id: strips to a dotted service-identity
key, which builtins only held in underscore form.
- name: the TraceQL span-name intrinsic, alongside duration and kind.
A file that does not parse falls back to the raw-text scan, which checks
every query rather than skipping it; JSON validity is already enforced by
the prettier pre-commit hook.
Adds 10 tests: the exemption, per-query scoping in a mixed dashboard,
target-inherits-panel datasource, no sideways inheritance leak, nested
row panels, TraceQL intrinsics, the malformed-JSON fallback, and the
collector-promotion helper.
This commit is contained in:
Pratik Mankawde
161
.github/scripts/otel-naming/check_otel_naming.py
vendored
161
.github/scripts/otel-naming/check_otel_naming.py
vendored
@@ -45,7 +45,8 @@ Layers
|
||||
include/**
|
||||
L2 collector : docker/telemetry/otel-collector-config.yaml (spanmetrics dims)
|
||||
L3 tempo : docker/telemetry/tempo.yaml (span filter tags)
|
||||
L4 dashboards: docker/telemetry/grafana/dashboards/*.json (PromQL labels)
|
||||
L4 dashboards: docker/telemetry/grafana/dashboards/*.json (PromQL labels;
|
||||
Loki/LogQL queries exempt -- see LOG_QUERY_DATASOURCES)
|
||||
L5 runbook : docs/telemetry-runbook.md (attr tables)
|
||||
L6 metrics : MetricsRegistry.cpp instrument labels (native-metric
|
||||
label keys, a valid dashboard-label source besides L1)
|
||||
@@ -65,7 +66,11 @@ Rules (each FAILS the build, when its inputs are present)
|
||||
C Every tempo span-filter tag exists in the L1 key set.
|
||||
D Every dashboard label resolves to an L1 span attribute, an L6
|
||||
native-metric label, or a builtin. TraceQL `span.`/`resource.` scope
|
||||
prefixes are stripped before the L1 lookup.
|
||||
prefixes are stripped before the L1 lookup. Log-datasource (Loki/LogQL)
|
||||
queries are exempt: their labels are minted by collector `regex_parser`
|
||||
captures or an in-query `| regexp` stage, so they have no L1/L6 source to
|
||||
resolve against (see LOG_QUERY_DATASOURCES). The exemption is per QUERY,
|
||||
not per file, so a mixed dashboard still has its PromQL panels checked.
|
||||
E No dotted `xrpl.<domain>.<field>` attribute key in the runbook (only the
|
||||
L1 resource attrs xrpl.network.* and the EXTERNAL_INFRA_LABELS dotted
|
||||
form -- xrpl.work.item/.branch/.node.role -- may be dotted). Span names,
|
||||
@@ -84,6 +89,7 @@ Exit code is non-zero if any present-and-enforced rule finds a violation.
|
||||
Warnings never change the exit code.
|
||||
"""
|
||||
|
||||
import json
|
||||
import re
|
||||
import subprocess
|
||||
import sys
|
||||
@@ -863,6 +869,100 @@ EXTERNAL_INFRA_LABELS = {
|
||||
}
|
||||
|
||||
|
||||
# Datasource types whose query language draws its label names from the log
|
||||
# stream at query time rather than from anything this repo's OTel code emits.
|
||||
#
|
||||
# A LogQL label has a third provenance Rule D cannot model: it is minted either
|
||||
# by the collector's `regex_parser` named capture groups (partition, severity —
|
||||
# see otel-collector-config.yaml) or by a `| regexp` stage inside the query
|
||||
# itself (action, pk, state, mode, phase, jobname, ip, pubkey). Both create the
|
||||
# label at ingest/query time, so no L1 (*SpanNames.h) or L6 (MetricsRegistry)
|
||||
# lookup can ever resolve them — checking them against those layers reports a
|
||||
# violation for a label that is correct by construction.
|
||||
#
|
||||
# This skips the QUERY, not the file: a dashboard mixing Prometheus and Loki
|
||||
# panels still has its Prometheus panels validated.
|
||||
LOG_QUERY_DATASOURCES = {"loki"}
|
||||
|
||||
# JSON keys that hold a query string (`expr` on panel targets, `query` on
|
||||
# template variables).
|
||||
QUERY_KEYS = ("expr", "query")
|
||||
|
||||
|
||||
def datasource_type(node: object) -> Optional[str]:
|
||||
"""Return the lowercased `datasource.type` of a dashboard node, if any.
|
||||
|
||||
Grafana writes the datasource either as an object (`{"type": "loki",
|
||||
"uid": "..."}`) or, on template variables, as a bare uid string. Only the
|
||||
object form carries the type, which is what identifies the query language.
|
||||
"""
|
||||
if isinstance(node, dict):
|
||||
ds = node.get("datasource")
|
||||
if isinstance(ds, dict):
|
||||
ds_type = ds.get("type")
|
||||
if isinstance(ds_type, str):
|
||||
return ds_type.lower()
|
||||
return None
|
||||
|
||||
|
||||
def iter_dashboard_queries(node: object, inherited: Optional[str] = None):
|
||||
"""Yield `(query_string, datasource_type)` for every query in a dashboard.
|
||||
|
||||
`datasource_type` is the query's own datasource when it declares one, else
|
||||
the nearest enclosing one (a panel's datasource covers its targets), else
|
||||
None. Walks the whole tree so panels nested inside collapsed rows are not
|
||||
missed.
|
||||
"""
|
||||
if isinstance(node, dict):
|
||||
current = datasource_type(node) or inherited
|
||||
for key in QUERY_KEYS:
|
||||
value = node.get(key)
|
||||
if isinstance(value, str) and value:
|
||||
yield value, current
|
||||
for value in node.values():
|
||||
yield from iter_dashboard_queries(value, current)
|
||||
elif isinstance(node, list):
|
||||
for value in node:
|
||||
yield from iter_dashboard_queries(value, inherited)
|
||||
|
||||
|
||||
def promoted_resource_labels(root: Path) -> Set[str]:
|
||||
"""Return resource attributes the collector promotes to metric labels.
|
||||
|
||||
The spanmetrics connector's `resource_metrics_key_attributes` lists the
|
||||
resource attributes copied onto every derived metric datapoint, where the
|
||||
prometheus exporter rewrites dots to underscores. So `deployment.environment`
|
||||
in the collector config becomes the `deployment_environment` label a
|
||||
dashboard filters on. Both forms are returned: the underscore form for
|
||||
PromQL, the dotted form for TraceQL `resource.<key>` references.
|
||||
|
||||
Derived from the config rather than hardcoded, so adding a key there is
|
||||
picked up automatically -- the same no-allowlist principle as Rules B and C.
|
||||
"""
|
||||
path = root / "docker" / "telemetry" / "otel-collector-config.yaml"
|
||||
if not path.is_file():
|
||||
return set()
|
||||
try:
|
||||
text = read_source(path)
|
||||
except OSError:
|
||||
return set()
|
||||
labels: Set[str] = set()
|
||||
in_block = False
|
||||
for line in text.splitlines():
|
||||
if re.search(r"\bresource_metrics_key_attributes\s*:", line):
|
||||
in_block = True
|
||||
continue
|
||||
if in_block:
|
||||
m = re.match(r"\s*-\s*([A-Za-z0-9_.]+)\s*$", line)
|
||||
if m:
|
||||
key = m.group(1)
|
||||
labels.add(key.replace(".", "_"))
|
||||
labels.add(key)
|
||||
elif line.strip() and not line.lstrip().startswith("-"):
|
||||
in_block = False
|
||||
return labels
|
||||
|
||||
|
||||
def run_rule_d_dashboards(
|
||||
root: Path, l1_keys: Set[str], metric_labels: Set[str], report: Report
|
||||
) -> None:
|
||||
@@ -886,23 +986,59 @@ def run_rule_d_dashboards(
|
||||
"job",
|
||||
"job_type", # standard Prometheus label for job-queue metrics
|
||||
"instance",
|
||||
# TraceQL intrinsics: fields of the span itself rather than attributes,
|
||||
# so they have no *SpanNames.h entry. `name` is the span name, matched
|
||||
# as `{name="consensus.accept"}`.
|
||||
"name",
|
||||
"duration",
|
||||
"kind",
|
||||
# Dotted form of the service identity, as TraceQL writes it
|
||||
# (`resource.service.instance.id` -> stripped to `service.instance.id`).
|
||||
"service.instance.id",
|
||||
"service.name",
|
||||
"service.version",
|
||||
}
|
||||
# A dashboard label is valid if it is a span attribute (L1), a native-metric
|
||||
# label (L6), a Prometheus/Grafana builtin, or an external-infra identity
|
||||
# label (EXTERNAL_INFRA_LABELS).
|
||||
valid = l1_keys | metric_labels | builtins | EXTERNAL_INFRA_LABELS
|
||||
# label (L6), a resource attribute the collector promotes onto metrics (L2),
|
||||
# a Prometheus/Grafana/TraceQL builtin, or an external-infra identity label
|
||||
# (EXTERNAL_INFRA_LABELS).
|
||||
valid = (
|
||||
l1_keys
|
||||
| metric_labels
|
||||
| promoted_resource_labels(root)
|
||||
| builtins
|
||||
| EXTERNAL_INFRA_LABELS
|
||||
)
|
||||
found = False
|
||||
checked = 0
|
||||
skipped_log_queries = 0
|
||||
for f in files:
|
||||
try:
|
||||
text = read_source(f)
|
||||
except OSError:
|
||||
continue
|
||||
# PromQL `sum by (a, b)` and `{label="..."}` references.
|
||||
# Parse so each query can be attributed to its datasource. Anything that
|
||||
# is not a well-formed dashboard object falls back to scanning the raw
|
||||
# text, which cannot attribute a datasource and so checks every query.
|
||||
# JSON validity itself is already enforced by the prettier pre-commit
|
||||
# hook, so this is a fallback and not a silent pass.
|
||||
try:
|
||||
queries = list(iter_dashboard_queries(json.loads(text)))
|
||||
except ValueError:
|
||||
queries = [(text, None)]
|
||||
labels: Set[str] = set()
|
||||
for m in re.finditer(r"by\s*\(([^)]*)\)", text):
|
||||
labels |= {x.strip() for x in m.group(1).split(",") if x.strip()}
|
||||
for m in re.finditer(r"\b([a-z_][a-z0-9_.]*)\s*[=!]~?\s*\"", text):
|
||||
labels.add(m.group(1))
|
||||
for query, ds_type in queries:
|
||||
if ds_type in LOG_QUERY_DATASOURCES:
|
||||
skipped_log_queries += 1
|
||||
continue
|
||||
checked += 1
|
||||
# PromQL `sum by (a, b)` and `{label="..."}` references. The
|
||||
# raw-text fallback still carries JSON's backslash-escaped quotes,
|
||||
# so accept an optional backslash before the quote.
|
||||
for m in re.finditer(r"by\s*\(([^)]*)\)", query):
|
||||
labels |= {x.strip() for x in m.group(1).split(",") if x.strip()}
|
||||
for m in re.finditer(r"\b([a-z_][a-z0-9_.]*)\s*[=!]~?\s*\\?\"", query):
|
||||
labels.add(m.group(1))
|
||||
for lbl in sorted(labels):
|
||||
# Strip a TraceQL scope prefix (span./resource./...) — the bare
|
||||
# attribute is what must resolve against L1.
|
||||
@@ -917,7 +1053,10 @@ def run_rule_d_dashboards(
|
||||
"must exist in L1, a metric label, or be a builtin",
|
||||
)
|
||||
if not found:
|
||||
report.ok(f"D: dashboard PromQL labels all resolve ({len(files)} file(s))")
|
||||
note = f"D: dashboard PromQL labels all resolve ({len(files)} file(s), {checked} query(s)"
|
||||
if skipped_log_queries:
|
||||
note += f", {skipped_log_queries} log query(s) skipped"
|
||||
report.ok(note + ")")
|
||||
|
||||
|
||||
def run_rule_e_runbook(root: Path, l1_keys: Set[str], report: Report) -> None:
|
||||
|
||||
@@ -17,6 +17,7 @@ filenames, OTel-standard keys, and metric labels — is the subtlest.
|
||||
import contextlib
|
||||
import importlib.util
|
||||
import io
|
||||
import json
|
||||
import shutil
|
||||
import tempfile
|
||||
import unittest
|
||||
@@ -811,6 +812,133 @@ class RuleDDashboards(unittest.TestCase):
|
||||
["span.not_a_key"],
|
||||
)
|
||||
|
||||
# ----- log-datasource (Loki/LogQL) exemption -----
|
||||
|
||||
@staticmethod
|
||||
def _dashboard(panels):
|
||||
"""Serialize a minimal well-formed dashboard around the given panels."""
|
||||
return json.dumps({"panels": panels})
|
||||
|
||||
@staticmethod
|
||||
def _panel(ds_type, expr):
|
||||
return {
|
||||
"type": "timeseries",
|
||||
"datasource": {"type": ds_type, "uid": "U1"},
|
||||
"targets": [{"datasource": {"type": ds_type, "uid": "U1"}, "expr": expr}],
|
||||
}
|
||||
|
||||
def test_loki_query_labels_exempt(self):
|
||||
# LogQL labels come from a collector regex_parser capture or an in-query
|
||||
# `| regexp` stage, so they have no L1/L6 source and must not be flagged.
|
||||
expr = 'sum by (partition, severity) ({service_name="xrpld"} | regexp `(?P<state>\\w+)`)'
|
||||
self.assertEqual(
|
||||
self._run(self._dashboard([self._panel("loki", expr)]), {"command"}), []
|
||||
)
|
||||
|
||||
def test_prometheus_query_still_checked_in_same_file(self):
|
||||
# The exemption is per query, not per file: a Prometheus panel beside a
|
||||
# Loki one is still validated.
|
||||
panels = [
|
||||
self._panel("loki", 'sum by (partition) ({service_name="xrpld"})'),
|
||||
self._panel("prometheus", "sum by (bogus_label) (rate(x[5m]))"),
|
||||
]
|
||||
self.assertEqual(
|
||||
self._run(self._dashboard(panels), {"command"}), ["bogus_label"]
|
||||
)
|
||||
|
||||
def test_target_inherits_panel_datasource(self):
|
||||
# A target with no datasource of its own inherits the panel's, so its
|
||||
# LogQL labels are still exempt.
|
||||
panel = {
|
||||
"type": "timeseries",
|
||||
"datasource": {"type": "loki", "uid": "U1"},
|
||||
"targets": [{"expr": 'sum by (partition) ({service_name="xrpld"})'}],
|
||||
}
|
||||
self.assertEqual(self._run(self._dashboard([panel]), {"command"}), [])
|
||||
|
||||
def test_loki_exemption_does_not_leak_to_sibling_panel(self):
|
||||
# A Loki panel must not make a later panel's datasource-less query
|
||||
# exempt -- inheritance flows down the tree, never sideways.
|
||||
panels = [
|
||||
self._panel("loki", 'sum by (partition) ({service_name="xrpld"})'),
|
||||
{"type": "timeseries", "targets": [{"expr": "sum by (leaked_label) (x)"}]},
|
||||
]
|
||||
self.assertEqual(
|
||||
self._run(self._dashboard(panels), {"command"}), ["leaked_label"]
|
||||
)
|
||||
|
||||
def test_nested_row_panel_is_walked(self):
|
||||
# Panels nested inside a collapsed row must still be checked.
|
||||
row = {
|
||||
"type": "row",
|
||||
"panels": [self._panel("prometheus", "sum by (nested_bogus) (x)")],
|
||||
}
|
||||
self.assertEqual(
|
||||
self._run(self._dashboard([row]), {"command"}), ["nested_bogus"]
|
||||
)
|
||||
|
||||
def test_traceql_intrinsics_not_flagged(self):
|
||||
# `name` is the span-name intrinsic and `resource.service.instance.id`
|
||||
# strips to a dotted service-identity key; neither is in *SpanNames.h.
|
||||
expr = '{name="consensus.accept" && resource.service.instance.id=~"$node"}'
|
||||
self.assertEqual(
|
||||
self._run(self._dashboard([self._panel("tempo", expr)]), {"command"}), []
|
||||
)
|
||||
|
||||
def test_malformed_json_falls_back_to_raw_scan(self):
|
||||
# A fragment cannot be attributed to a datasource, so every query is
|
||||
# checked rather than silently skipped.
|
||||
self.assertEqual(
|
||||
self._run('"expr": "sum by (fragment_bogus) (x)"', {"command"}),
|
||||
["fragment_bogus"],
|
||||
)
|
||||
|
||||
|
||||
class PromotedResourceLabels(unittest.TestCase):
|
||||
"""L2: resource attrs the collector promotes onto metric datapoints."""
|
||||
|
||||
def _run(self, collector_text):
|
||||
d = Path(tempfile.mkdtemp())
|
||||
try:
|
||||
_write(
|
||||
d / "docker" / "telemetry" / "otel-collector-config.yaml",
|
||||
collector_text,
|
||||
)
|
||||
return chk.promoted_resource_labels(d)
|
||||
finally:
|
||||
shutil.rmtree(d)
|
||||
|
||||
def test_both_forms_returned(self):
|
||||
# The prometheus exporter rewrites dots to underscores, and TraceQL uses
|
||||
# the dotted form, so a listed key must validate either way.
|
||||
labels = self._run(
|
||||
" resource_metrics_key_attributes:\n"
|
||||
" - service.instance.id\n"
|
||||
" - deployment.environment\n"
|
||||
" - xrpl.network.type\n"
|
||||
)
|
||||
self.assertIn("deployment_environment", labels)
|
||||
self.assertIn("deployment.environment", labels)
|
||||
self.assertIn("xrpl_network_type", labels)
|
||||
|
||||
def test_block_ends_at_next_key(self):
|
||||
# A sibling key ends the list; its values must not become labels.
|
||||
labels = self._run(
|
||||
" resource_metrics_key_attributes:\n"
|
||||
" - deployment.environment\n"
|
||||
" histogram:\n"
|
||||
" unit: ms\n"
|
||||
)
|
||||
self.assertIn("deployment_environment", labels)
|
||||
self.assertNotIn("unit", labels)
|
||||
self.assertNotIn("ms", labels)
|
||||
|
||||
def test_absent_config_returns_empty(self):
|
||||
# Presence-gated, like every other rule input.
|
||||
self.assertEqual(
|
||||
chk.promoted_resource_labels(Path(tempfile.gettempdir()) / "nope"), set()
|
||||
)
|
||||
|
||||
|
||||
class MetricLabelExtraction(unittest.TestCase):
|
||||
"""L6: native-metric label keys parsed from C++ instrument calls."""
|
||||
|
||||
Reference in New Issue
Block a user