feat(observability): initialize OpenTelemetry pipeline and system-wide observability#107
feat(observability): initialize OpenTelemetry pipeline and system-wide observability#107taitelee wants to merge 4 commits into
Conversation
github-actions
Bot
added
documentation
PR title doesn't match the required formatGot: Expected format: Allowed types: The optional Examples:
This check is required by the |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request lays the groundwork for comprehensive system observability by integrating OpenTelemetry across the application. It enables detailed tracing, structured logging, and metric collection, providing a unified view of system behavior. The changes facilitate easier debugging and performance monitoring in both distributed and standalone environments, ensuring that the lifecycle of requests and messages can be tracked from end-to-end, even across asynchronous boundaries like NATS queues. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 comprehensive observability stack using OpenTelemetry and SigNoz, including structured logging with slog, distributed tracing, and metrics. It adds several ClickHouse configuration files, a new clickhouse_json_bridge for Bento, and detailed documentation for the observability setup. However, several critical issues were identified: a potential compilation error due to an undeclared Binary variable, a violation of the style guide regarding r.Context() usage in the query handler, and a security risk involving an unbounded read of HTTP response bodies. Additionally, the new ingest path lacks unit tests, and several documentation and configuration syncs are missing. Please address these issues to ensure system stability and maintainability.
| logLevel.Set(level) | ||
|
|
||
| baseLogger := slog.New(slog.NewJSONHandler(os.Stdout, &slog.HandlerOptions{Level: logLevel, AddSource: true})) | ||
| logger := baseLogger.With("version", Version, "build_time", BuildTime, "git_commit", GitCommit, "binary", Binary) |
There was a problem hiding this comment.
[MUST] The variable Binary is used here but is not declared in the var block or anywhere else in the package. This will cause a compilation error. Please declare it along with Version, BuildTime, and GitCommit.
| logger := baseLogger.With("version", Version, "build_time", BuildTime, "git_commit", GitCommit, "binary", Binary) | |
| logger := baseLogger.With("version", Version, "build_time", BuildTime, "git_commit", GitCommit) |
| // Execute query with singleflight to protect ClickHouse from thundering herds. | ||
| v, err, _ := h.sf.Do(cacheKey, func() (interface{}, error) { | ||
| queryCtx, cancel := context.WithTimeout(r.Context(), 30*time.Second) | ||
| detachedCtx := context.WithoutCancel(r.Context()) |
There was a problem hiding this comment.
[MUST] Using context.WithoutCancel(r.Context()) explicitly ignores the client's cancellation signal. This violates the repository style guide rule: "HTTP handlers ignoring r.Context()". While this is likely intended to prevent singleflight from aborting shared work, it can lead to resource leaks (e.g., ClickHouse queries continuing to run after all clients have disconnected). Consider using r.Context() directly or a more sophisticated singleflight pattern that honors individual request cancellations.
References
- HTTP handlers ignoring r.Context() is a correctness violation. (link)
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode >= 300 { | ||
| body, _ := io.ReadAll(resp.Body) |
There was a problem hiding this comment.
[MUST] io.ReadAll(resp.Body) performs an unbounded read into memory. If the ClickHouse server returns a large error response, this could lead to an Out-Of-Memory (OOM) condition. Use io.LimitReader to constrain the amount of data read.
| body, _ := io.ReadAll(resp.Body) | |
| body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024*1024)) // Limit to 1MB |
|
|
||
| // Attach Stack Trace for Errors | ||
| if r.Level >= slog.LevelError { | ||
| r.AddAttrs(slog.String("stacktrace", string(debug.Stack()))) |
There was a problem hiding this comment.
[SHOULD] Calling debug.Stack() on every error log is expensive and can significantly impact performance in high-throughput scenarios. Consider making stack trace collection optional or using a more efficient way to capture context.
|
|
||
| if err := service.RegisterBatchOutput("clickhouse_json_bridge", service.NewConfigSpec(), | ||
| func(conf *service.ParsedConfig, mgr *service.Resources) (service.BatchOutput, service.BatchPolicy, int, error) { | ||
| return &clickhouseOutput{ |
There was a problem hiding this comment.
[MUST] The new clickhouseOutput component and its batching logic lack unit tests. Per the repository style guide, new code on critical paths like ingest must include tests.
References
- New code on ingest paths requires tests.
| ``` | ||
| **2. Copy the necessary files into WaveHouse** | ||
| ```bash | ||
| mkdir -p ~/BeachHouse/observability |
There was a problem hiding this comment.
[MUST] The project name is incorrectly referenced as BeachHouse. It should be WaveHouse for consistency across documentation.
| mkdir -p ~/BeachHouse/observability | |
| mkdir -p ~/WaveHouse/observability |
| trace.WithBatcher(traceExporter, trace.WithBatchTimeout(time.Second*5)), | ||
| trace.WithResource(res), | ||
| trace.WithSampler(trace.TraceIDRatioBased(0.10)), // TRACE SAMPLE RATE | ||
| trace.WithSampler(trace.TraceIDRatioBased(0.10)), // TRACE SAMPLE RATE COMMENTED OUT FOR TESTING |
There was a problem hiding this comment.
[MAY] The comment "TRACE SAMPLE RATE COMMENTED OUT FOR TESTING" is misleading because the sampler is actually active. Please update the comment to reflect the current state.
| trace.WithSampler(trace.TraceIDRatioBased(0.10)), // TRACE SAMPLE RATE COMMENTED OUT FOR TESTING | |
| trace.WithSampler(trace.TraceIDRatioBased(0.10)), // 10% sampling rate |
| DLQ DLQ `yaml:"dlq"` | ||
| Policy Policy `yaml:"policy"` | ||
| Pipes Pipes `yaml:"pipes"` | ||
| Observability Observability `yaml:"observability"` |
There was a problem hiding this comment.
[MUST] New configuration fields and environment variables (WH_OBSERVABILITY_ENABLED, WH_OTEL_ADDR) have been added. These must be documented in docs/configuration.md, docs/deployment.md, and CHANGELOG.md. Additionally, per repository rules, new configuration fields should be validated to prevent invalid values, such as empty strings, which could cause runtime issues.
References
- New configuration fields should be validated to prevent invalid values, such as empty strings, which could cause runtime issues.
- Notable API changes, such as new configuration fields, must be documented in CHANGELOG.md and the relevant API documentation.
- Ensure all related documentation files are updated consistently when configuration is changed.
| "github.com/Wave-RF/WaveHouse/internal/observability" | ||
| "go.opentelemetry.io/otel" | ||
|
|
||
| "go.opentelemetry.io/otel/attribute" // ADD THIS |
There was a problem hiding this comment.
[MAY] Please remove the "// ADD THIS" comment as it appears to be a leftover from development. Iterate.
| "go.opentelemetry.io/otel/attribute" // ADD THIS | |
| "go.opentelemetry.io/otel/attribute" |
github-actions
Bot
removed
dependencies
github-actions
Bot
added
dependencies
) ## Summary Second-order fix on top of #105. The author-trust + required-check gate in `claude-review.yml` now uses the REST `check-runs` endpoint instead of GraphQL's `statusCheckRollup` field on the PR view. ## Why `gh pr view --json statusCheckRollup` asks GraphQL to navigate `checkSuite.workflowRun` on every check context on the PR's head commit. The workflow's `GITHUB_TOKEN` is not allowed to read that sub-field for checks owned by other integrations (Gemini Code Assist, Copilot, third-party statuses). When a PR has enough cross-integration checks the whole call fails with: ``` GraphQL: Resource not accessible by integration (...statusCheckRollup.contexts.nodes.5) (...nodes.0..4.checkSuite.workflowRun) ``` We don't actually use any of that traversal — the gate just needs `name`, `status`, `conclusion` for two named checks (`CI`, `PR housekeeping`). The REST endpoint returns exactly that and nothing else, so it doesn't trip the cross-integration permission issue. This failure was always there — #105 fixing `authorAssociation` just let the script reach the next bug. Visible on PR #88 (long-lived, many integrations) but not PR #7 (fewer integrations), which is why we didn't catch it in initial testing. ## What changed - `gh pr view --json` drops `statusCheckRollup`; keeps `isDraft`, `author`, `headRefOid`. - New `gh api repos/$REPO/commits/$head_sha/check-runs?per_page=100` call for the rollup, only inside the `workflow_run` branch (other event paths bypass the snapshot like before). - Status / conclusion comparisons updated from uppercase (GraphQL enum: `COMPLETED`, `SUCCESS`, ...) to lowercase (REST enum: `completed`, `success`, ...). - Gate behavior is otherwise identical: same set of pass-through conclusions, same skip notices, same fall-through to the Claude action when CI is green. ## Verified `gh api repos/Wave-RF/WaveHouse/commits/a4f5a6b1cc83ffdca5429c0d72fb593f3f232eda/check-runs` (PR #88's current head) returns both `CI` and `PR housekeeping` with `status=completed, conclusion=success`. No GraphQL errors. ## Out of scope (mentioned in investigation, not changed here) - `observability` branch and `context_aware_mq_2` branch still carry pre-#89 workflow files (`board-state-sync.yml`, `claude-agent.yml`) and a pre-#77 runner label. Rebase those branches onto current `main` to drop them. - The \"Claude\" run on `context_aware_mq_2` (event `dynamic`, runs on `ubuntu-24.04`) is GitHub Copilot Coding Agent in Claude mode, not our workflow file. - PR #107 (observability) `PR housekeeping` failures are legitimate — the PR title exceeds 72 chars. ## Test plan - [ ] Next workflow_run for PR #88 (or any internal PR with cross-integration checks) — the gate completes without GraphQL errors. - [ ] Draft / Dependabot / untrusted-author PRs still skip with their respective notices. - [ ] A green CI on a trusted-author PR still falls through to the Claude review step. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
This PR establishes the core observability infrastructure for WaveHouse. By integrating OpenTelemetry (OTel) and a structured
sloglogger, we now have a unified pipeline for traces, metrics, and logs. This foundation allows for deep inspection of system performance and simplifies debugging in distributed or standalone environments.Changes made:
internal/observabilityto initialize the OTel Trace and Metric providers, supporting exports to OTLP-compatible backends (like SigNoz or Jaeger).sloghandler that automatically attaches Trace IDs to log entries when a span is active.wavehousebinary to initialize the observability pipeline on boot based on theobservabilityconfig block.Test plan
internal/observabilityinitialization and logger formatting via existing suite.WH_OTEL_ADDR.otelShutdownis called on SIGINT/SIGTERM to prevent spans from being dropped during process exit.Related Issues
Closes #