Skip to content

Commit 52d2789

Browse files
authored
Merge pull request #49 from trick77/review/splunk-logs-followups
review: follow-ups to #48 (env default, contextvars unbind, schema tests)
2 parents b8b739c + 712a566 commit 52d2789

7 files changed

Lines changed: 324 additions & 9 deletions

File tree

AGENTS.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ If `docker ps` fails, ask the user to start OrbStack.
5353
- **One JSON object per line, on stdout.** OpenShift's Splunk Connect for Kubernetes (SCK) tails the container log; Splunk auto-extracts fields via `KV_MODE=json` for sourcetype `riptide:collector:json` (set as pod annotation in `openshift/collector/deployment.yaml`).
5454
- **Stdlib loggers (uvicorn, sqlalchemy, alembic) are bridged through structlog.** Do NOT add separate logging handlers or re-init `logging.basicConfig``configure_logging()` in `logging_config.py` is the single entry point.
5555
- **Splunk-reserved field names are forbidden as kwargs**: `source`, `sourcetype`, `host`, `index`, `time`, `_time`, `_raw`, `event`. The CI vendor field is `ci_system` (not `source`); the structlog event name lives in `msg` (renamed from `event`); severity lives in `log_level` (renamed from `level`). A runtime processor (`_strip_reserved`) namespaces accidental reserved kwargs under `splunk_<name>` as a safety net — do not rely on it; pick the right name from the start.
56+
- **Field-naming convention** for non-reserved kwargs: prefer generic names that mean the same across sources (`event_type`, `status`, `phase`, `delivery_id`, `team`, `repo`, `commit_sha`). Don't pre-namespace with the source name (`noergler_event_type`, `pipeline_status`) — `webhook_source` already disambiguates in `stats by webhook_source, event_type`. Only namespace when two sources legitimately mean different things by the same word and would collide in a single Splunk panel.
5657
- **Webhook handlers emit exactly one `msg=webhook_processed` log per request** with required fields `webhook_source ∈ {bitbucket,pipeline,argocd,noergler}`, `outcome ∈ {accepted,deduped,ignored,skipped}`, `delivery_id`, `team`. Source-specific fields go alongside (e.g. `app`, `revision`, `phase` for argocd). Include `delivery_id` even on `ignored`/`skipped` paths so triage has a key.
5758
- **`outcome=deduped`** is detected via `RETURNING delivery_id` on the `INSERT ... ON CONFLICT DO NOTHING` — a `None` scalar means the row already existed. Preserve this when adding new sources.
5859
- **Persist failures**: wrap the `async with session_factory()` block in `try/except Exception: logger.exception("webhook_persist_failed", ...); raise`. Never swallow.

openshift/collector/configmap-app.yaml

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,10 @@ metadata:
44
name: riptide-collector-app
55
data:
66
RIPTIDE_LOG_LEVEL: "INFO"
7-
# Stamped on every log record as `env`. Override per overlay (intg / prod).
8-
RIPTIDE_ENV: "prod"
7+
# Stamped on every log record as `env`. Each overlay MUST patch this to
8+
# the real environment ("intg" / "prod"). The "unknown" sentinel makes a
9+
# missing patch loud at first glance in Splunk rather than mis-tagging.
10+
RIPTIDE_ENV: "unknown"
911
RIPTIDE_CONFIG_PATH: "/etc/riptide-collector/riptide.json"
1012
RIPTIDE_TEAM_KEYS_PATH: "/etc/riptide-collector-team-keys/team-keys.json"
1113
RIPTIDE_CONFIG_RELOAD_SECONDS: "30"

openshift/collector/deployment.yaml

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,9 @@ spec:
1919
app.kubernetes.io/name: riptide-collector
2020
app.kubernetes.io/component: collector
2121
annotations:
22-
# Splunk Connect for Kubernetes reads these per-pod annotations.
22+
# Splunk Connect for Kubernetes reads this per-pod annotation.
2323
# The sourcetype must have `KV_MODE=json` configured in props.conf
24-
# so Splunk auto-extracts our JSON log fields. The index annotation
25-
# can be overridden per overlay.
24+
# so Splunk auto-extracts our JSON log fields.
2625
splunk.com/sourcetype: "riptide:collector:json"
2726
spec:
2827
securityContext:

src/riptide_collector/main.py

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,15 +87,21 @@ async def access_log( # pyright: ignore[reportUnusedFunction]
8787
) -> Response:
8888
# Liveness/readiness checks fire every few seconds; logging them
8989
# buries real traffic in Splunk. Pass through unobserved.
90-
if request.url.path in _SILENT_PATHS:
90+
# rstrip handles `/health/` (trailing slash) too.
91+
if request.url.path.rstrip("/") in _SILENT_PATHS:
9192
return await call_next(request)
9293
request_id = request.headers.get("x-request-id") or uuid.uuid4().hex
94+
bound = ("request_id", "method", "path")
9395
structlog.contextvars.bind_contextvars(
9496
request_id=request_id,
9597
method=request.method,
9698
path=request.url.path,
9799
)
98100
started = time.perf_counter()
101+
# Sentinel for the unhandled-exception path: if call_next raises
102+
# before we overwrite status_code, the finally block still emits
103+
# a numeric status. FastAPI's default exception handler turns the
104+
# exception into a real 500 response after the middleware unwinds.
99105
status_code = 500
100106
try:
101107
response: Response = await call_next(request)
@@ -107,7 +113,9 @@ async def access_log( # pyright: ignore[reportUnusedFunction]
107113
status_code=status_code,
108114
duration_ms=round((time.perf_counter() - started) * 1000, 1),
109115
)
110-
structlog.contextvars.clear_contextvars()
116+
# Unbind only what we bound — don't nuke contextvars set by
117+
# callers up the stack (lifespan, future auth layers).
118+
structlog.contextvars.unbind_contextvars(*bound)
111119

112120
app.include_router(health.make_router(config, session_factory, team_keys, any_auth))
113121
app.include_router(bitbucket.make_router(config, session_factory, bitbucket_hmac))

src/riptide_collector/routers/noergler.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ async def noergler_webhook( # pyright: ignore[reportUnusedFunction]
6262
webhook_source="noergler",
6363
outcome="accepted" if inserted is not None else "deduped",
6464
delivery_id=values["delivery_id"],
65-
noergler_event_type=values["event_type"],
65+
event_type=values["event_type"],
6666
team=caller_team,
6767
)
6868
return {"status": "accepted"}

src/riptide_collector/routers/pipeline.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ async def pipeline_webhook( # pyright: ignore[reportUnusedFunction]
8080
pipeline=event.pipeline_name,
8181
run_id=event.run_id,
8282
phase=event.phase,
83-
run_status=event.status,
83+
status=event.status,
8484
team=caller_team,
8585
)
8686
return {"status": "accepted"}

tests/test_log_schema.py

Lines changed: 305 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,305 @@
1+
"""Schema regression tests for the structured log output.
2+
3+
Why these exist: PR #48 unified all webhook handlers onto one
4+
`msg=webhook_processed` event with a fixed kwarg vocabulary and forbids
5+
Splunk-reserved field names on the wire. Without these tests the schema
6+
silently drifts on the next add-a-source / add-a-field PR — which is
7+
exactly what the unification was supposed to end.
8+
9+
Two layers are covered:
10+
11+
1. **Call-site invariants** (per router): capture stdout via `capfd` and
12+
parse the JSON lines. This is what Splunk actually sees, so it's the
13+
right thing to assert against. (`structlog.testing.capture_logs` was
14+
tried first but its global-config swap doesn't reach loggers cached
15+
under `cache_logger_on_first_use=True`, which silently breaks across
16+
tests.)
17+
2. **Processor invariants** (one test class): run the configured processor
18+
chain on a synthetic event_dict and assert `event→msg`, `level→log_level`,
19+
reserved-name stripping, and `service/version/env` stamping.
20+
"""
21+
22+
from __future__ import annotations
23+
24+
import io
25+
import json
26+
import logging
27+
from collections.abc import Iterator
28+
from pathlib import Path
29+
from typing import Any, cast
30+
31+
import pytest
32+
import structlog
33+
from httpx import AsyncClient
34+
35+
from _keys import CHECKOUT_NOERGLER
36+
from riptide_collector import __version__
37+
from riptide_collector.logging_config import configure_logging
38+
39+
FIXTURES = Path(__file__).parent / "fixtures"
40+
41+
42+
def _load(name: str) -> dict[str, Any]:
43+
return json.loads((FIXTURES / name).read_text())
44+
45+
46+
def _parse_json_lines(stream_text: str) -> list[dict[str, Any]]:
47+
"""Return JSON-object lines from a captured log stream, ignoring
48+
non-JSON noise (httpx debug lines, etc.)."""
49+
out: list[dict[str, Any]] = []
50+
for raw in stream_text.splitlines():
51+
line = raw.strip()
52+
if not line.startswith("{"):
53+
continue
54+
try:
55+
out.append(json.loads(line))
56+
except json.JSONDecodeError:
57+
continue
58+
return out
59+
60+
61+
def _processed(events: list[dict[str, Any]]) -> list[dict[str, Any]]:
62+
return [e for e in events if e.get("msg") == "webhook_processed"]
63+
64+
65+
@pytest.fixture
66+
def log_buffer(client: AsyncClient) -> Iterator[io.StringIO]:
67+
"""Swap every root StreamHandler's stream to an in-memory buffer for
68+
the test. Depends on `client` so it runs AFTER `configure_logging`
69+
has installed the JSON handler.
70+
71+
Why not capfd/capsys: the structlog handler captures `sys.stdout`
72+
at handler-creation time. Once create_app runs in the client fixture,
73+
that reference is frozen — pytest's later fd/sys redirection doesn't
74+
reach it. Swapping `handler.stream` directly is the only reliable
75+
way to test what Splunk would actually see.
76+
"""
77+
del client # ordering dependency only
78+
buf = io.StringIO()
79+
root = logging.getLogger()
80+
saved: list[tuple[logging.StreamHandler[Any], Any]] = []
81+
for handler in root.handlers:
82+
if isinstance(handler, logging.StreamHandler):
83+
saved.append((handler, handler.stream))
84+
handler.stream = buf
85+
try:
86+
yield buf
87+
finally:
88+
for handler, original in saved:
89+
handler.stream = original
90+
91+
92+
@pytest.fixture
93+
def log_buffer_ignored(
94+
client_with_ignored_stages: AsyncClient,
95+
) -> Iterator[io.StringIO]:
96+
del client_with_ignored_stages
97+
buf = io.StringIO()
98+
root = logging.getLogger()
99+
saved: list[tuple[logging.StreamHandler[Any], Any]] = []
100+
for handler in root.handlers:
101+
if isinstance(handler, logging.StreamHandler):
102+
saved.append((handler, handler.stream))
103+
handler.stream = buf
104+
try:
105+
yield buf
106+
finally:
107+
for handler, original in saved:
108+
handler.stream = original
109+
110+
111+
# ---------- call-site invariants ----------
112+
113+
114+
class TestWebhookProcessedSchema:
115+
async def test_pipeline_accepted_then_deduped(
116+
self, client: AsyncClient, log_buffer: io.StringIO
117+
) -> None:
118+
payload = _load("pipeline_jenkins_completed.json")
119+
headers = {"Authorization": "Bearer test-checkout-jenkins-bearer"}
120+
121+
r1 = await client.post("/webhooks/pipeline", json=payload, headers=headers)
122+
r2 = await client.post("/webhooks/pipeline", json=payload, headers=headers)
123+
124+
assert r1.status_code == 202
125+
assert r2.status_code == 202
126+
events = _processed(_parse_json_lines(log_buffer.getvalue()))
127+
assert len(events) == 2
128+
129+
first, second = events
130+
for ev in events:
131+
for key in (
132+
"msg",
133+
"log_level",
134+
"timestamp",
135+
"service",
136+
"version",
137+
"env",
138+
"webhook_source",
139+
"outcome",
140+
"delivery_id",
141+
"team",
142+
):
143+
assert key in ev, f"missing {key} in {ev}"
144+
# Splunk-reserved field names must not appear on the wire.
145+
for forbidden in ("source", "event", "level", "host", "index", "sourcetype"):
146+
assert forbidden not in ev, f"reserved {forbidden} leaked in {ev}"
147+
assert ev["webhook_source"] == "pipeline"
148+
149+
assert first["outcome"] == "accepted"
150+
assert second["outcome"] == "deduped"
151+
assert first["ci_system"] == "jenkins"
152+
# Generic field names, not pipeline-namespaced.
153+
assert "status" in first
154+
assert "run_id" in first
155+
156+
async def test_argocd_accepted_then_deduped(
157+
self, client: AsyncClient, log_buffer: io.StringIO
158+
) -> None:
159+
payload = _load("argocd_synced.json")
160+
headers = {"Authorization": "Bearer test-checkout-argocd-bearer"}
161+
162+
r1 = await client.post("/webhooks/argocd", json=payload, headers=headers)
163+
r2 = await client.post("/webhooks/argocd", json=payload, headers=headers)
164+
165+
assert r1.status_code == 202
166+
assert r2.status_code == 202
167+
events = _processed(_parse_json_lines(log_buffer.getvalue()))
168+
assert len(events) == 2
169+
assert events[0]["outcome"] == "accepted"
170+
assert events[1]["outcome"] == "deduped"
171+
for ev in events:
172+
assert ev["webhook_source"] == "argocd"
173+
assert ev["delivery_id"]
174+
assert "source" not in ev
175+
176+
async def test_argocd_ignored_carries_delivery_id(
177+
self,
178+
client_with_ignored_stages: AsyncClient,
179+
log_buffer_ignored: io.StringIO,
180+
) -> None:
181+
payload = _load("argocd_synced.json")
182+
# The canonical fixture targets prod, which the ignored-stages
183+
# fixture leaves alone. Force a stage the fixture covers.
184+
payload["destination_namespace"] = "payments-api-syst"
185+
headers = {"Authorization": "Bearer test-checkout-argocd-bearer"}
186+
187+
r = await client_with_ignored_stages.post("/webhooks/argocd", json=payload, headers=headers)
188+
189+
assert r.status_code == 202
190+
events = _processed(_parse_json_lines(log_buffer_ignored.getvalue()))
191+
assert len(events) == 1
192+
ev = events[0]
193+
assert ev["outcome"] == "ignored"
194+
assert ev["reason"] == "stage_in_ignored_stages"
195+
# Regression guard: argocd_event_ignored used to omit delivery_id,
196+
# leaving triage with no key to grep on.
197+
assert ev["delivery_id"]
198+
assert "environment" in ev
199+
200+
async def test_noergler_uses_generic_event_type_field(
201+
self, client: AsyncClient, log_buffer: io.StringIO
202+
) -> None:
203+
payload = _load("noergler_completed.json")
204+
headers = {"Authorization": f"Bearer {CHECKOUT_NOERGLER}"}
205+
206+
r = await client.post("/webhooks/noergler", json=payload, headers=headers)
207+
208+
assert r.status_code == 202
209+
events = _processed(_parse_json_lines(log_buffer.getvalue()))
210+
assert len(events) == 1
211+
ev = events[0]
212+
assert ev["webhook_source"] == "noergler"
213+
# Field is `event_type`, not `noergler_event_type` — webhook_source
214+
# already disambiguates in Splunk panels.
215+
assert ev["event_type"] == "completed"
216+
assert "noergler_event_type" not in ev
217+
218+
219+
class TestHttpRequestLog:
220+
async def test_request_id_header_propagates(
221+
self, client: AsyncClient, log_buffer: io.StringIO
222+
) -> None:
223+
r = await client.get(
224+
"/auth/ping",
225+
headers={
226+
"Authorization": "Bearer test-checkout-argocd-bearer",
227+
"X-Request-Id": "test-rid-abc",
228+
},
229+
)
230+
assert r.status_code == 200
231+
232+
events = _parse_json_lines(log_buffer.getvalue())
233+
http = [e for e in events if e.get("msg") == "http_request"]
234+
assert len(http) == 1
235+
ev = http[0]
236+
assert ev["request_id"] == "test-rid-abc"
237+
assert ev["path"] == "/auth/ping"
238+
assert ev["method"] == "GET"
239+
assert ev["status_code"] == 200
240+
assert isinstance(ev["duration_ms"], (int, float))
241+
242+
async def test_health_path_not_logged(
243+
self, client: AsyncClient, log_buffer: io.StringIO
244+
) -> None:
245+
for _ in range(3):
246+
await client.get("/health")
247+
events = _parse_json_lines(log_buffer.getvalue())
248+
assert [e for e in events if e.get("msg") == "http_request"] == []
249+
250+
251+
# ---------- processor-chain invariants ----------
252+
253+
254+
class TestProcessorChain:
255+
def _run_chain(self, env: str, **kwargs: Any) -> dict[str, Any]:
256+
configure_logging("INFO", env=env)
257+
event_dict: dict[str, Any] = {"event": "x", "level": "info", **kwargs}
258+
processed: Any = event_dict
259+
from riptide_collector.logging_config import (
260+
_make_service_metadata_processor,
261+
_rename_level,
262+
_strip_reserved,
263+
)
264+
265+
chain = [
266+
_make_service_metadata_processor(env),
267+
structlog.processors.EventRenamer("msg"),
268+
_rename_level,
269+
_strip_reserved,
270+
]
271+
for proc in chain:
272+
processed = proc(cast(Any, None), "info", processed)
273+
assert isinstance(processed, dict)
274+
return processed
275+
276+
def test_event_renamed_to_msg_level_to_log_level(self) -> None:
277+
out = self._run_chain("prod")
278+
assert out["msg"] == "x"
279+
assert out["log_level"] == "info"
280+
assert "event" not in out
281+
assert "level" not in out
282+
283+
def test_service_metadata_stamped(self) -> None:
284+
out = self._run_chain("intg")
285+
assert out["service"] == "riptide-collector"
286+
assert out["version"] == __version__
287+
assert out["env"] == "intg"
288+
289+
def test_splunk_reserved_kwargs_namespaced(self) -> None:
290+
# Anyone accidentally passing `source=...` or `host=...` as a kwarg
291+
# would be silently overwritten by Splunk's input metadata. The
292+
# `_strip_reserved` safety net moves them under `splunk_<name>`.
293+
out = self._run_chain(
294+
"prod",
295+
source="jenkins",
296+
host="x",
297+
index="y",
298+
sourcetype="z",
299+
)
300+
assert out["splunk_source"] == "jenkins"
301+
assert out["splunk_host"] == "x"
302+
assert out["splunk_index"] == "y"
303+
assert out["splunk_sourcetype"] == "z"
304+
for forbidden in ("source", "host", "index", "sourcetype"):
305+
assert forbidden not in out

0 commit comments

Comments
 (0)