feat(observability): add structured tags to all 10 counters (#35)#38
Conversation
Brainstorm output for the 6 open GitHub issues. v1.1 scope = #34/#35/#36 (refactor + structured tags + observability docs); #13/#14/#23 deferred to v1.2. Three sequential PRs against main: #35 → #36 (hard-ordered), #34 in any slot (independent). PROJECT.md gains a "Post-v1.0 Scope — v1.1" section. ROADMAP.md gains Phase 13 (matches Phase 11/12 style; 60 lines). HISTORY.md records the brainstorm dialogue. STATE.json restored from HEAD (working tree had a stale Phase 4 snapshot) and forward-rolled to phase 13 / ready. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 plans / 18 tasks across 3 waves: - Wave 1 (PR #35): PLAN-1.1 helper-method refactor + PLAN-1.2 MeterListener tests - Wave 2 (PR #36): PLAN-2.1 docs/dashboard + PLAN-2.2 compose stacks/Makefile + PLAN-2.3 drift test/RELEASING/CHANGELOG - Wave 3 (PR #34): PLAN-3.1 BlueIrisUrlTemplate collapse + canonical-set test Discussion captured 4 design decisions (D1-D4) + 4 OQ resolutions (D5-D8 + OQ-2/4) in CONTEXT-13.md. RESEARCH.md (336 lines) documented every counter call site, OTLP wiring, template-class shapes. Architect generated all plans first dispatch. Verifier completeness PASS_WITH_NOTES (3 caveats addressed inline); critique READY. Plan artifacts in .shipyard/phases/13/ are intentionally local-only per .shipyard/.gitignore. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…th XML doc-comments Adds one static helper per counter that builds the tag array internally, so all tag key strings are declared only in DispatcherDiagnostics.cs (D1). Updates each counter field doc-comment with canonical tags, cardinality rule, and drift hint to docs/observability.md. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…atcherDiagnostics helpers Replaces three inline .Add() calls with IncrementEventsReceived, IncrementEventsMatched, and IncrementErrorsUnhandled. EventPump now holds no tag-string literals; IncrementErrorsUnhandled adds the previously missing "component" tag. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…DispatcherDiagnostics helpers PLAN-1.1 Task 2. Replaces inline KeyValuePair[] arrays at every counter increment site (events.matched, actions.dispatched/succeeded/failed, validators.passed/rejected, dispatch.drops, dispatch.exhausted) with calls to the per-counter helpers introduced in 14ec2f5. EventPump migrated in d53c4a1. The eviction callback's Drops.Add becomes IncrementDrops(evicted, "channel_full") per CONTEXT-13 OQ-2 — the callback already has the full DispatchItem in scope. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ase 9 D9 → Phase 13 #35) The Phase-9-era test ErrorsUnhandled_*_Untagged asserted errors.unhandled emitted with no tags ("intentionally tagless (D3/D9)"). Phase 13 issue #35 explicitly tags the counter with `component` so operators can triage unhandled errors by failing subsystem (EventPump vs dispatcher vs plugin). Renamed to _TaggedWithComponent and assertion updated. User confirmed Phase 13 wins over the prior Phase 9 design intent during build-time decision check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-presence tests One test per counter in DispatcherDiagnostics; each directly invokes the Increment* helper with sentinel values and asserts exact tag key set + tag values. Forbidden tag `event_id` tripwire included on every test. Closes the tag-matrix acceptance criteria for Phase 13 PLAN-1.2 Task 1. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Documents that all 10 Meter "FrigateRelay" counters now emit structured tags and that errors.unhandled gains a component tag replacing the deliberately tagless Phase 9 behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughPhase 13 advances Shipyard to v1.1 with observability focus. Structured tags are added to all FrigateRelay counters via new ChangesObservability Infrastructure & Tagging
Sequence DiagramsequenceDiagram
participant EventPump
participant ChannelActionDispatcher
participant DispatcherDiagnostics
participant Meter as Counter/Meter<br/>(FrigateRelay)
EventPump->>DispatcherDiagnostics: IncrementEventsReceived(context)
DispatcherDiagnostics->>Meter: Add(1, tags: camera, label)
EventPump->>DispatcherDiagnostics: IncrementEventsMatched(context, sub)
DispatcherDiagnostics->>Meter: Add(1, tags: subscription, camera, label)
EventPump->>ChannelActionDispatcher: EnqueueAsync(event)
ChannelActionDispatcher->>DispatcherDiagnostics: IncrementActionsDispatched(item)
DispatcherDiagnostics->>Meter: Add(1, tags: subscription, camera, action)
ChannelActionDispatcher->>ChannelActionDispatcher: Validate(item)
ChannelActionDispatcher->>DispatcherDiagnostics: IncrementValidatorsPassed/Rejected(item, name)
DispatcherDiagnostics->>Meter: Add(1, tags: subscription, camera, validator, action)
ChannelActionDispatcher->>ChannelActionDispatcher: ExecuteAsync(plugin)
alt Success
ChannelActionDispatcher->>DispatcherDiagnostics: IncrementActionsSucceeded(item)
DispatcherDiagnostics->>Meter: Add(1, tags: subscription, camera, action)
else Exhausted
ChannelActionDispatcher->>DispatcherDiagnostics: IncrementExhausted(item)
DispatcherDiagnostics->>Meter: Add(1, tags: subscription, camera, action)
else Failed
ChannelActionDispatcher->>DispatcherDiagnostics: IncrementActionsFailed(item)
DispatcherDiagnostics->>Meter: Add(1, tags: subscription, camera, action)
end
alt Channel Full
ChannelActionDispatcher->>DispatcherDiagnostics: IncrementDrops(item, "channel_full")
DispatcherDiagnostics->>Meter: Add(1, tags: subscription, camera, reason)
end
EventPump->>DispatcherDiagnostics: IncrementErrorsUnhandled("EventPump")
DispatcherDiagnostics->>Meter: Add(1, tags: component)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 41 minutes and 31 seconds.Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/FrigateRelay.Host.Tests/Observability/CounterIncrementTests.cs (1)
202-207:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale section comment contradicts the updated test.
The section header at line 203 still reads
"with NO tags"but the test was renamed and the assertion now verifiescomponent = "EventPump". Anyone reading the comment before the assertion body will be misled.✏️ Proposed fix
- // Test 8: errors.unhandled increments ONCE with NO tags when IEventSource throws + // Test 8: errors.unhandled increments ONCE with component="EventPump" tag when IEventSource throws🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/FrigateRelay.Host.Tests/Observability/CounterIncrementTests.cs` around lines 202 - 207, Update the stale test section header comment that currently says "with NO tags" to reflect the test's new behavior: change the header to indicate the error is tagged with the component (e.g., "errors.unhandled increments ONCE when IEventSource throws, TAGGED with component 'EventPump'") or remove the misleading phrase; ensure this comment sits immediately above the test method ErrorsUnhandled_Increments_OnPumpFault_Once_TaggedWithComponent so it accurately describes the assertion that the counter is tagged with component = "EventPump".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.shipyard/ROADMAP.md:
- Around line 415-417: Update the "Phase Count" section to reflect the new Phase
13: change the line that reads "**12 phases.** Within the 8–14 range requested.
Phases 1–2 parallelizable after the `.sln` exists; Phases 3–12 sequential." to
state "**13 phases.** Within the 8–15 range requested. Phases 1–2 parallelizable
after the `.sln` exists; Phases 3–13 sequential." so the total count and the
sequential phase range both match the added phase.
---
Outside diff comments:
In `@tests/FrigateRelay.Host.Tests/Observability/CounterIncrementTests.cs`:
- Around line 202-207: Update the stale test section header comment that
currently says "with NO tags" to reflect the test's new behavior: change the
header to indicate the error is tagged with the component (e.g.,
"errors.unhandled increments ONCE when IEventSource throws, TAGGED with
component 'EventPump'") or remove the misleading phrase; ensure this comment
sits immediately above the test method
ErrorsUnhandled_Increments_OnPumpFault_Once_TaggedWithComponent so it accurately
describes the assertion that the counter is tagged with component = "EventPump".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3412a27e-f740-41f8-aa2a-05225f166766
📒 Files selected for processing (11)
.shipyard/HISTORY.md.shipyard/PROJECT.md.shipyard/ROADMAP.md.shipyard/STATE.json.shipyard/STATE.json.sha256CHANGELOG.mdsrc/FrigateRelay.Host/Dispatch/ChannelActionDispatcher.cssrc/FrigateRelay.Host/Dispatch/DispatcherDiagnostics.cssrc/FrigateRelay.Host/EventPump.cstests/FrigateRelay.Host.Tests/Observability/CounterIncrementTests.cstests/FrigateRelay.Host.Tests/Observability/CounterTagMatrixTests.cs
- ROADMAP.md "Phase Count" updated 12 → 13 to reflect the new v1.1 Observability + Cleanup phase (Phase 13). - CounterIncrementTests.cs Test 8 section header updated to match the renamed `_TaggedWithComponent` test and its component=EventPump assertion (Phase 13 #35 supersedes Phase 9's tagless behavior). Both stale-text findings; no behavioral or test-coverage changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR #38 (issue #35 counter tags) merged as b110bb3. Local main rebased clean. Wave 2 worktree branched at .worktrees/feat-36-observability-docs off origin/main; baseline 239/239 tests, 0 warnings. Next: dispatch builders for PLAN-2.1 / 2.2 / 2.3 (PR #36 scope). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes #35.
Summary
Meter "FrigateRelay"now emit structured tags so dashboards can pivot bysubscription,camera,action,validator,reason, andcomponent. Tag matrix matches the inventory in Add structured tags (camera/action/subscription) to FrigateRelay meters #35 with one structural amendment captured in.shipyard/phases/13/CONTEXT-13.mdD5:events.receivedis tagged withcamera+labelonly (the increment site is pre-subscription-match; tagging bysubscriptionwould have changed the counter's semantic meaning).internal static void Increment*helpers onDispatcherDiagnosticsso every call site is a one-liner and a future tag drift becomes a single-file edit. The eviction callback inChannelActionDispatcherconsumes the full evictedDispatchItem(no callback-shape change). Per-counter XML doc-comments list the tag set + cardinality rule + the "if you add a counter here, updatedocs/observability.md" reminder.tests/FrigateRelay.Host.Tests/Observability/CounterTagMatrixTests.csadds 10MeterListener-based tag-presence tests (one per counter). Each test asserts the expected tag KEYS, that VALUES match the test-scenario sentinels, and thatevent_idis absent — a per-test cardinality-bomb tripwire so a future contributor adding an 11th counter test cannot silently omit the rule.ErrorsUnhandled_*_Untagged(which assertederrors.unhandledcarried no tags) is renamed_TaggedWithComponentand updated to assert the newcomponent=EventPumptag. Operators gain per-subsystem error triage; Phase 9's deliberately-tagless decision is intentionally overruled.[Unreleased]### Addedentry.docs/observability.mdis referenced as forthcoming (lands in PR Document metrics + add sample Grafana dashboard (docs/observability.md) #36 / Wave 2 of Phase 13).What this is NOT
docs/observability.md,docker/observability/and the Grafana dashboard JSON ship in PR Document metrics + add sample Grafana dashboard (docs/observability.md) #36 (Wave 2 of Phase 13).Test plan
ubuntu-latestandwindows-latest(Linux + Windows warnings-as-errors gate).dotnet build FrigateRelay.sln -c Releasereports 0 warnings, 0 errors locally.bash .github/scripts/run-tests.sh --skip-integrationreports 239 tests, 0 failures (up from 229 baseline = +10 fromCounterTagMatrixTests).git grep '"event_id"' src/FrigateRelay.Host/Dispatch/returns empty (CONTEXT-13 hard rule:event_idis the cardinality-bomb tag and must never appear at an increment site).git grep -nE 'App\.Metrics|OpenTracing|Jaeger\.' src/still empty (architectural invariant unchanged).Follow-ups
plugin.Namefrom loop variable. Pre-existing closure-capture pattern surfaced by this refactor; counter tags themselves are correct. One-line fix in a follow-up PR.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
componenttag to unhandled error metrics to better categorize failures by subsystem.Documentation
Tests