Blind review: production observability call sites - #403
Conversation
WalkthroughИзменения расширяют observability для daemon lifecycle, gRPC transport/auth/version flows, фоновой индексации и database initialization. Добавлены контрактные тесты, OTLP resource identity, MeterProvider accessor и артефакты успешных test/review-прогонов. ChangesИнтеграция наблюдаемости
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.12.2)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 focus:behavioral correctness, lifecycle concurrency, security and low-cardinality telemetry, release regressions |
|
@codex review |
|
I'll flag anything relevant to auth, version, database, and index diagnostics call sites specifically, given the objectives noted for this blind review. ✅ Action performedReview finished.
|
![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 integrates OpenTelemetry gRPC instrumentation (otelgrpc) across client and server connections, separates daemon and server resource identities, and adds bounded runtime event tracking for authentication, version negotiation, database initialization, and background indexing. Additionally, daemon observability lifecycle management is added to the main entry point and graceful restart paths. Feedback is provided regarding a potential issue where the telemetry context is cancelled prematurely immediately after initialization, which could abort background connection attempts in the OTLP exporter.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| telemetryCtx, telemetryCancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| telemetry, err := initDaemonObservability(telemetryCtx) | ||
| telemetryCancel() | ||
| if err != nil { | ||
| logger.Error("observability initialization failed", "error", err) | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
Cancelling the telemetryCtx immediately after initDaemonObservability returns can abort background connection and retry attempts in the OTLP exporter. Since otlpmetricgrpc.New initializes the connection asynchronously in the background, the context passed to it must remain active for the connection to be established and maintained.
Passing context.Background() directly avoids premature cancellation of the exporter's background connection.
telemetry, err := initDaemonObservability(context.Background())
if err != nil {
logger.Error("observability initialization failed", "error", err)
os.Exit(1)
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/engram/main.go (1)
513-513: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winЛогируйте исходную ошибку shutdown.
Сейчас лог не содержит
err, поэтому нельзя отличить timeout, TLS-ошибку и сбой exporter.Предлагаемое исправление
- logger.Error("observability shutdown failed; check collector availability") + logger.Error("observability shutdown failed; check collector availability", "error", err)🤖 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 `@cmd/engram/main.go` at line 513, Update the observability shutdown error handling around the logger.Error call to include the original shutdown err alongside the existing context message. Preserve the current shutdown failure path while ensuring the logged details distinguish timeout, TLS, and exporter errors.
🤖 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
@.agent/reports/evidence/production-ready/observability/sol-synthesis-20260713-r1/summary.json:
- Around line 23-27: Update the evidence generation in verify-otlp.ps1 so
process_residue and container_residue are populated from actual process and
container checks rather than hard-coded empty arrays, and make verdict fail when
either contains residue. If these checks cannot be implemented, remove both
fields from the evidence contract and stop claiming their absence.
In
@.agent/reports/evidence/production-ready/release-gates-foundation/critical-suite-runner/sol-synthesis-20260713-observability-r2/commands.json:
- Around line 18-19: Normalize the evidence command records around the command
metadata and stdout/stderr artifact references: replace host-specific absolute
paths with repository-relative paths, explicitly record the working_directory,
and apply the same change to the additionally referenced entries. Preserve
stable artifact identification while ensuring commands are portable across hosts
and do not expose local machine structure.
---
Nitpick comments:
In `@cmd/engram/main.go`:
- Line 513: Update the observability shutdown error handling around the
logger.Error call to include the original shutdown err alongside the existing
context message. Preserve the current shutdown failure path while ensuring the
logged details distinguish timeout, TLS, and exporter errors.
🪄 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: 23a76fab-a0c3-4857-aede-6956e783c2ed
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (25)
.agent/reports/evidence/production-ready/observability/sol-synthesis-20260713-r1/summary.json.agent/reports/evidence/production-ready/release-gates-foundation/critical-suite-runner/sol-synthesis-20260713-observability-r2/commands.json.agent/reports/evidence/production-ready/release-gates-foundation/critical-suite-runner/sol-synthesis-20260713-observability-r2/go-test-summary.json.agent/reports/evidence/production-ready/release-gates-foundation/critical-suite-runner/sol-synthesis-20260713-observability-r2/summary.json.agent/reviews/2026-07-13-ops-observability-callsites-sol-review.md.agent/specs/ops-observability-call-sites/evidence/OBS-CALLSITES-R1.tdd.jsoncmd/engram/main.gocmd/engram/observability_contract_test.gogo.modinternal/grpcserver/observability_contract_test.gointernal/grpcserver/server.gointernal/grpcserver/version_negotiate.gointernal/handlers/codeintel/module.gointernal/handlers/codeintel/observability_contract_test.gointernal/handlers/engramcore/grpcpool.gointernal/handlers/engramcore/observability_contract_test.gointernal/handlers/serverevents/bridge.gointernal/handlers/serverevents/observability_contract_test.gointernal/module/obs/meter.gointernal/module/obs/metrics.gointernal/module/obs/runtime.gointernal/module/obs/runtime_test.gointernal/worker/database_observability_contract_test.gointernal/worker/service.goscripts/production-smoke/verify-otlp.ps1
| "missing_tests": [], | ||
| "failed_tests": [], | ||
| "process_residue": [], | ||
| "container_residue": [], | ||
| "verdict": "PASS" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Не заявляйте отсутствие residue без фактической проверки.
process_residue и container_residue всегда записываются как пустые массивы в scripts/production-smoke/verify-otlp.ps1, а verdict не учитывает утечки процессов или контейнеров. Поэтому этот PASS-артефакт может пройти при оставшемся exporter/collector residue. Добавьте реальную проверку либо удалите эти поля из evidence-контракта.
🤖 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
@.agent/reports/evidence/production-ready/observability/sol-synthesis-20260713-r1/summary.json
around lines 23 - 27, Update the evidence generation in verify-otlp.ps1 so
process_residue and container_residue are populated from actual process and
container checks rather than hard-coded empty arrays, and make verdict fail when
either contains residue. If these checks cannot be implemented, remove both
fields from the evidence contract and stop claiming their absence.
| "stdout": "D:\\Dev\\engram\\.agent\\worktrees\\prc-sol-synthesis-r1\\.agent\\reports\\evidence\\production-ready\\release-gates-foundation\\critical-suite-runner\\sol-synthesis-20260713-observability-r2\\go-test.stdout.jsonl", | ||
| "stderr": "D:\\Dev\\engram\\.agent\\worktrees\\prc-sol-synthesis-r1\\.agent\\reports\\evidence\\production-ready\\release-gates-foundation\\critical-suite-runner\\sol-synthesis-20260713-observability-r2\\go-test.stderr.log" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Нормализуйте пути в evidence-командах.
commands.json смешивает абсолютные пути D:\Dev\engram\... с относительным -InputPath, не фиксируя рабочий каталог. На другом хосте команда будет непереносимой, а артефакт раскрывает локальную структуру рабочей машины. Сохраняйте repo-relative пути и явно записывайте working_directory, либо используйте стабильные идентификаторы артефактов.
Also applies to: 27-34, 40-41
🤖 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
@.agent/reports/evidence/production-ready/release-gates-foundation/critical-suite-runner/sol-synthesis-20260713-observability-r2/commands.json
around lines 18 - 19, Normalize the evidence command records around the command
metadata and stdout/stderr artifact references: replace host-specific absolute
paths with repository-relative paths, explicitly record the working_directory,
and apply the same change to the additionally referenced entries. Preserve
stable artifact identification while ensuring commands are portable across hosts
and do not expose local machine structure.
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Review-only PR for exact commit 888708b over d6b3375. Do not merge. Judge production behavior, OTel lifecycle concurrency, daemon-vs-shim ownership, auth/version/database/index diagnostics, low-cardinality security, and test sensitivity. Full focused/full/critical evidence is committed in the diff.
Summary by CodeRabbit
Новые возможности
Исправления