Skip to content

review: follow-ups to #48 (env default, contextvars unbind, schema tests)#49

Merged
trick77 merged 1 commit into
masterfrom
review/splunk-logs-followups
May 17, 2026
Merged

review: follow-ups to #48 (env default, contextvars unbind, schema tests)#49
trick77 merged 1 commit into
masterfrom
review/splunk-logs-followups

Conversation

@trick77

@trick77 trick77 commented May 17, 2026

Copy link
Copy Markdown
Owner

Summary

Addresses the code-review feedback on the merged PR #48 (Splunk-friendly JSON logs):

  • RIPTIDE_ENV default in openshift/collector/configmap-app.yaml flipped from "prod" to "unknown". A forgotten overlay patch now stands out in Splunk instead of silently mis-tagging records as production.
  • Stale comment in openshift/collector/deployment.yaml about overridable index annotation dropped — we never set that annotation.
  • access_log middleware (src/riptide_collector/main.py):
    • _SILENT_PATHS check uses rstrip("/") so /health/ (trailing slash) is also silenced.
    • clear_contextvars() replaced with unbind_contextvars(*bound) — we only unbind what we bound, preserving any contextvars set by an outer layer.
    • Added a sentinel comment on the status_code = 500 fallback so a future reader doesn't misread it as a bug.
  • Field-naming convention aligned across routers:
    • noergler_event_typeevent_type in routers/noergler.py.
    • run_statusstatus in routers/pipeline.py.
    • webhook_source already disambiguates these in Splunk panels; source-namespaced field names are no longer used.
  • AGENTS.md documents the convention so the next add-a-source PR doesn't reinvent it.
  • tests/test_log_schema.py (new): the schema-regression suite the unification was supposed to enable. Two layers:
    • Call-site (per router): swap the root StreamHandler's stream to an in-memory buffer (capfd/capsys don't reach handlers that captured sys.stdout before the test started), parse the JSON lines, assert required keys, outcome semantics, no Splunk-reserved leaks, accepted→deduped on replay, delivery_id even on ignored paths, request_id propagation, /health silence.
    • Processor-chain: run the prod processor list on a synthetic event_dict and assert event→msg, level→log_level, reserved-name stripping, and service/version/env stamping.

Test plan

  • uv run ruff check . && uv run ruff format --check .
  • uv run pyright — 0 errors
  • uv run pytest — 179 passed (170 baseline + 9 new schema tests)
  • On intg, confirm RIPTIDE_ENV overlay patch is in place (otherwise records will tag env=unknown)

…chema tests)

- configmap: default RIPTIDE_ENV to "unknown" instead of "prod" so a
  missing overlay patch is loud in Splunk rather than silently tagged
  as production.
- deployment: drop stale "index annotation can be overridden" comment
  that referenced an annotation we never set.
- access_log middleware: rstrip path before checking _SILENT_PATHS so
  /health/ doesn't sneak through; replace clear_contextvars() with
  unbind_contextvars(*bound) so we only unbind what we bound (preserves
  contextvars set by any outer layer); add a sentinel comment on the
  500 fallback.
- noergler/pipeline routers: align log field names with the
  cross-source convention — `event_type` (not noergler_event_type),
  `status` (not run_status). webhook_source already disambiguates in
  Splunk panels.
- AGENTS.md: document the naming convention so the next add-a-source
  PR doesn't reinvent it.
- tests/test_log_schema.py: add a regression suite the unification was
  supposed to make possible. Two layers:
  * call-site: parse the JSON the handler actually wrote (via a stream
    swap on the root StreamHandler — capfd/capsys don't reach handlers
    that captured sys.stdout before the test started) and assert
    required keys, outcome semantics, no Splunk-reserved leaks,
    accepted -> deduped on replay, delivery_id even on ignored paths,
    request_id propagation, /health silence.
  * processor-chain: run the prod processor list on a synthetic event
    dict and assert event->msg, level->log_level, reserved stripping,
    and service/version/env stamping.
@trick77 trick77 merged commit 52d2789 into master May 17, 2026
3 checks passed
@trick77 trick77 deleted the review/splunk-logs-followups branch May 17, 2026 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant