feat(obs,loom): Phase 2 — obs.MeterFor() helper + loom OTel wiring#168
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughДобавлен пакет Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a centralized observability utility for OpenTelemetry meters and integrates it into the loom module. Specifically, it adds the obs package with a MeterFor function to standardize instrumentation scope naming and updates the loom engine initialization to include metrics and a retry policy. Feedback suggests avoiding hardcoded strings by using module methods or shared constants and replacing magic numbers with named constants for better maintainability.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/handlers/loom/module.go (1)
121-121: ИспользуйтеmoduleNameвместо строкового литерала на Line 121.Сейчас
"loom"дублирует уже существующую константуmoduleName; лучше убрать риск расхождения при будущем переименовании модуля.Предлагаемое изменение
- loom.WithMeter(obs.MeterFor("loom")), + loom.WithMeter(obs.MeterFor(moduleName)),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handlers/loom/module.go` at line 121, Replace the hard-coded string "loom" passed to obs.MeterFor with the existing moduleName constant to avoid duplication; specifically update the call used with loom.WithMeter(obs.MeterFor("loom")) to loom.WithMeter(obs.MeterFor(moduleName)), ensuring moduleName is in scope where this call occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/handlers/loom/module.go`:
- Line 121: Replace the hard-coded string "loom" passed to obs.MeterFor with the
existing moduleName constant to avoid duplication; specifically update the call
used with loom.WithMeter(obs.MeterFor("loom")) to
loom.WithMeter(obs.MeterFor(moduleName)), ensuring moduleName is in scope where
this call occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1717f083-3799-4dff-bb64-f48f7bc6ec6a
📒 Files selected for processing (3)
internal/handlers/loom/module.gointernal/module/obs/meter.gointernal/module/obs/meter_test.go
🤖 PR Review MCP State (auto-managed, do not edit){
"version": 2,
"parentChildren": {},
"resolvedNitpicks": {
"coderabbit-nitpick-1803fc13-121": {
"resolvedAt": "2026-04-15T15:47:15.943Z",
"resolvedBy": "agent"
}
},
"updatedAt": "2026-04-15T15:47:16.489Z"
} |
Addresses CodeRabbit nitpick on PR #168 — replace hardcoded string literal with the existing moduleName package constant to avoid divergence if the module is ever renamed.
Phase 6 release ceremony for the v4.4.0 train. Bumps the unified engram-server + plugin version after the 5-PR train completed: #167 — Phase B-1 plumbing tenant (loom integration) #168 — Phase 2 obs.MeterFor helper + loom OTel wiring #169 — Phase 3 4 loom_* tools + CLI worker with allowlist #170 — Phase 4 server-side gRPC proto extensions + soft-delete reaper #171 — Phase 5 daemon serverevents bridge for real OnProjectRemoved e2e Per Constitution §15 the daemon version and plugin version must move together, so this commit bumps: - cmd/engram/main.go daemonVersion "v4.3.0" → "v4.4.0" Reported to gRPC Initialize + logged in structured output. - plugin/engram/.claude-plugin/plugin.json version "4.3.0" → "4.4.0" Drives Claude Code plugin cache invalidation on /reload-plugins. Without this bump the marketplace update would not be detected. No behaviour change, no test change. Release notes + git tag + gh release + marketplace sync land in subsequent steps.
1 participant
Summary
Wires loom's internal OTel instruments through the engram main meter pipeline. Implements spec FR-11 with the spec C1 Option B resolution: extend
internal/module/obs/with aMeterFor(moduleName) metric.Meterhelper rather than adding aModuleDeps.Meterframework field.What ships
internal/module/obs/meter.go(21 LOC) — newMeterFor(moduleName string) metric.Meterhelper wrappingotel.GetMeterProvider().Meter("github.com/thebtf/engram/" + moduleName). Single-point accessor; respects the framework rule "no code outsideinternal/module/obs/may callotel.GetMeterProvider()directly".internal/module/obs/meter_test.go(55 LOC) — 3 parallel unit tests:TestMeterFor_NonNil— non-nil meter for any valid nameTestMeterFor_SameNameSameScope— two calls with same name return usable factoriesTestMeterFor_EmptyName— empty name does not panicinternal/handlers/loom/module.go(+6/-1) —Initnow passesloom.WithMeter(obs.MeterFor("loom"))betweenWithLoggerandWithMaxRetriestoloom.NewEngine(db, opts...). Removes the earlier explanatory comment about NoopMeter fallback (no longer applicable).Why this approach
Spec C1 clarification chose Option B (
obs.MeterFor()helper) over Option A (newModuleDeps.Meterfield):internal/moduletest/mocks.goMeterForis the new seamotel.GetMeterProvider()is cached by the OTel libraryEffect on loom metrics
With this PR merged, loom's 8 internal OTel instruments emit through the same meter provider that serves
engram_handletool_duration_msand friends:loom.tasks.submitted(counter)loom.tasks.completed(counter)loom.tasks.failed(counter)loom.tasks.cancelled(counter)loom.gate.pass(counter)loom.gate.fail(counter)loom.submit.duration_ms(histogram)loom.task.duration_ms(histogram)All 8 carry
worker_typeandproject_idattributes from loom v0.1.0. Operators withOTEL_EXPORTER_OTLP_ENDPOINTconfigured will see them alongside the framework metrics; operators without an exporter pay zero cost (noop provider).Verification
go build ./...— cleango vet ./...— cleango test ./internal/module/obs/... -count=1— 3/3 new tests PASSgo test ./internal/handlers/loom/... -count=1— 8/8 existing loom tests PASS (no regression)Anti-stub check: replacing
MeterForbody withreturn nilwould failTestMeterFor_NonNil. Verified by reading the test and tracing the assertion.What does NOT change
RecordHandleTooletc.) — unchangedModuleDepsstruct — no new field (that was Option A, not taken)Related
.agent/specs/loom-integration/spec.mdFR-11 + C1 clarification (gitignored).agent/specs/loom-integration/plan.md§Phase 2 (gitignored).agent/specs/loom-integration/tasks.mdT010 + T011 + T012 (gitignored)Summary by CodeRabbit
Примечания к выпуску
Улучшения
Тесты