Repository navigation
feat(observability): add Loki log-sink provider (#662) - #1060
Conversation
Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe <vibe@mistral.ai> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ct (#662) The multi-service loki module now sets serviceName: loki on the provided log-sink contract, matching the marquez module and the contract field's own guidance, so the E150 host cross-check uses an explicit serving-service advertisement instead of relying only on the namesake-service rule. Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe <vibe@mistral.ai>
RonaldHensbergen
left a comment
RonaldHensbergen
left a comment
There was a problem hiding this comment.
Review verdict
- Blocking: none
- Major: none
- Minor: 1 — missing
serviceNameadvertisement on the providedlog-sinkcontract (inline comment above); fixed in e37f44a on this branch
Recommendation: merge after CI re-runs green on e37f44a (quality judgment, not just mergeable-status: tests, lint, CI, review substance all checked below).
What was verified
- CI (head 74fabdc): all check runs green — CI (lint, package, security-scan, cds-runtime, tests on ubuntu/windows/macos, CDS Test Stable Profile), Docker Runtime Smoke Test, GitHub Advanced Security, SonarCloud Code Analysis (quality gate passed, 0 new issues).
- Tests locally: full suite via
scripts/run_tests_with_deprecation_gate.py— 2048 tests OK (30 skipped);tests/test_loki_module.py4/4;module.yamlvalidates againstcli/resources/module.schema.json; yamllint, ruff (UP rules), markdownlint all clean on the changed files. - Render/plan/runtime: rendered a local profile with the module — loopback-only port
127.0.0.1:8686:8686, no docker.sock mount,read_only+cap_drop: ALL+pids_limit+ tmpfs on both services, retention env7d/90d,LOKI_ENDPOINTcorrectly namespaced tologs-loki-storage:3100, named volumelogs-loki-data.cds testpasses all four stages (validate/security/plan/render). - Live runtime smoke (Docker, pinned digests): started both services with the module's exact flags; POSTed structured (all five string fields), raw, and wrong-typed events to
/logs— all HTTP 200; Loki streams show{tier="structured"}only for the fully-stringed event and{tier="raw"}for the other two; Loki acceptedretention_period: 7d+retention_stream90d override and reached ready. So this does not just render green — the classification and retention wiring works at runtime. - Security posture:
no-new-privilegesomission is empirically justified, not a shortcut — both pinned binaries failexecwith EPERM underno_new_privsin this runc runtime (tested as root and non-root; binaries are not setuid, consistent with file capabilities). All other hardening (cap_drop ALL, read_only, pids_limit, tmpfs, loopback-only publish,auth_enabled: falsedocumented as local-only,productionSuitable: false) is in place; unauthenticated ingest + no container-log tailing are documented in the module README known-limitations. CDS-SEC-090/091 don't apply to local-classified profiles with a non-production-suitable module; the security stage passes. Tracked upstream follow-up: #1066. - Repo conventions: CHANGELOG
[Unreleased]→ Added entry present in house style; docs updated (docs/observability.md§5,common-ground-alignment.md,haven-parity-plan.mdrow 3.3);docs/roadmap.mdalready lists the module (#1057). No new E-/W-codes and nocli/changes, so no README troubleshooting rows or code-collision checks needed; PR carries no labels per convention; single Conventional-Commit feat commit with co-author trailer.
Out of scope, filed as issues (milestone inherited from #662)
- #1067 — repo-wide contract-port semantics (host-published port advertised as the in-network contract port; breaks in-network consumers when
config.portis non-default) - #1065 — docker smoke-test CI does not cover experimental modules; loki/otel runtime behavior is only manually verified
- #1066 — pinned vector/loki images fail exec under
no-new-privileges; re-enablesecurity_optonce upstream drops the file capabilities
Note: open PR #1063 (otel-collector) touches docs/observability.md, CHANGELOG.md, docs/haven-parity-plan.md — expect merge conflicts there, none functional.
|
Review follow-ups filed during review (out of scope for this PR, milestone inherited from #662):
The one in-PR minor finding (missing |
RonaldHensbergen
left a comment
RonaldHensbergen
left a comment
There was a problem hiding this comment.
End-to-end review complete. No actionable issues found. I reviewed the contract and endpoint wiring, Loki/Vector configuration, retention-tier classification, security posture and limitations, documentation, and tests. The pinned Loki and Vector configurations validate; the module tests and an isolated Docker smoke test pass, including confirming raw and structured records reach their respective Loki tiers. All PR checks are green. No open review points remain; in my judgment, this PR is merge-ready.
RonaldHensbergen
left a comment
RonaldHensbergen
left a comment
There was a problem hiding this comment.
Correction to my earlier verdict: I found one actionable issue that the default-port smoke test did not expose. ingestPort changes the loopback host binding only (host:8686 in the container), but the current contract exports it as the in-network port; setting a non-default host port therefore breaks Compose consumers. I fixed the local working copy and added a regression test using a non-default host port. Those changes are not in this PR head yet, so the PR is not merge-ready until they are included and CI reruns.
Keep the log-sink contract on Vector's container listener port while allowing the host loopback publish port to be configured independently. Cover the distinction with a non-default host port regression test. Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe <vibe@mistral.ai> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Summary
Validation
Closes #662