Conversation
imeyer
requested review from
MichaelUnkey,
chronark,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe update introduces a new configuration setup and telemetry framework for the service. A new file defines a Changes
Sequence Diagram(s)sequenceDiagram
participant M as Main
participant C as Config Loader
participant T as Telemetry Setup
participant S as HTTP Server
M->>C: LoadConfig() \n(read environment vars)
C-->>M: Return Config object
M->>T: setupTelemetry(ctx, Config)
T-->>M: Return TelemetryConfig & cleanup function
M->>S: Initialize & start HTTP Server \n(using config.ListenerPort)
S-->>M: Process incoming requests \n(with telemetry instrumentation)
Suggested reviewers
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 (1.62.2)Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24.0) Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
apps/chproxy/config.go (2)
22-31: LoadConfig defaults are well-structured.
Providing sensible defaults reduces misconfiguration risks. Consider documenting these defaults in the README or with doc comments for future maintainers.
33-36: Validate ClickhouseURL further if needed.
You return an error if it's empty, which is good. For robustness, you might also parse the URL to confirm it’s syntactically valid and log potential issues.apps/chproxy/otel.go (1)
39-40: GlobalcurrentBufferSizevariable.
Be aware of concurrency. Multiple goroutines might update or read this value concurrently; consider using atomic operations if data races occur.apps/chproxy/main.go (4)
8-8: Retaining standard log usage.
You’re still importing"log". Consider removing it if you have fully migrated toslog.
62-68: Request construction with OTEL context.
Good approach. Also, consider timeouts or adjustable client timeouts if large batches or high latencies are expected.
84-88: Network error metrics.
You accurately capture request failure. Consider custom attributes for root-cause analysis if networking issues are frequent.
217-218: Debug log for new batch type.
Helps observe query usage patterns. Mind potential PII if queries have sensitive data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/chproxy/go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
apps/chproxy/config.go(1 hunks)apps/chproxy/go.mod(1 hunks)apps/chproxy/main.go(5 hunks)apps/chproxy/otel.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Go API Local / test_agent_local
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Build / Build
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (40)
apps/chproxy/config.go (6)
1-2: No issues with package declaration.
No concerns here; usingpackage mainis standard for an executable entry point.
3-8: Imports are appropriate.
All imported packages are relevant and used. Ensure that indirect dependencies (e.g.,log/slog) remain stable if you bump Go versions.
10-20: Config struct design looks good.
The fields cover essential parameters for the application. Storing a*slog.Loggeris acceptable, but verify concurrency safety if you plan to update the logger at runtime.
38-41: BasicAuth retrieval is straightforward.
Storing the credential in plain text is typical for Basic Auth, but remain mindful of possible security repercussions if logs or error messages inadvertently leak secrets.
43-45: Dynamic ListenerPort is well-handled.
Changing the default port viaPORTenvironment variable is a common approach.
47-48: Config returned successfully.
Nothing to add here; returningconfig, nilis clean and conventional.apps/chproxy/otel.go (5)
1-2: Package declaration is clear.
No concerns; this file naturally belongs to the main package for the chproxy app.
3-21: Imports align well with OTEL integration.
All required OpenTelemetry packages are included. Confirm that the specified OTEL modules stay in sync with your Go module versions.
23-37: TelemetryConfig struct is comprehensive.
You encapsulate logging, metrics, and tracer objects. The nestedMetricsstruct fosters clarity. Good approach.
41-49: SetBufferSize / GetBufferSize are straightforward.
If concurrency usage is high, atomic operations or locks may be prudent. For now, it’s acceptable if the usage is minimal or single-threaded.
51-174: setupTelemetry function is robust.
- Properly sets up resources, metric exporters, logger providers, and tracer providers.
- Includes error checks for metrics creation and returns a cleanup function.
- Ensure each newly allocated provider (log, trace, metric) is properly shut down for memory stability.
Overall, this function is a strong foundation for observability.apps/chproxy/main.go (26)
17-19: New OTEL imports.
Using OTEL’sattribute,codes, andmetricfosters standardized tracing and metrics.
28-28: Global telemetry variable.
This is fine for an app-level reference. Just be sure to handle concurrency or reinitialization carefully if the application evolves.
36-36: Persist function signature extended.
Accepting bothcontext.Contextandconfig *Configis beneficial for passing OTEL context and config settings.
37-39: OTEL span on persist.
Starting a new span for each persisted batch is a good practice for detailed tracing.
44-45: Recording metrics for batch and row counts.
This helps measure data throughput. Good usage of counters.
47-50: Storing query text in a span attribute.
Be mindful that queries can contain sensitive data. Evaluate whether partial redaction is needed for compliance or privacy reasons.
52-57: Error handling logs metrics and sets OTEL error status.
Capturing error details in metrics and OTEL traces is valuable for troubleshooting. Nicely done.
73-78: Validating password in URL credentials.
Properly increments error metrics and sets trace status. This is thorough.
92-95: Success log for persisted rows.
Clear log message with row count and query info is helpful for debugging.
96-111: Comprehensive error logging on non-200 response.
Captures HTTP status code, response body, increments error metric, sets trace error. Good thoroughness.
116-119: LoadConfig error.
Terminating at fatal is appropriate if critical config details are missing.
124-135: Telemetry setup with cleanup function.
Using a deferred cleanup ensures your OTEL resources shut down gracefully. Good practice.
137-140: Logger setup from TelemetryConfig.
Ensures a consistent approach with new OTEL-based log handling.
142-145: Initial service announcement.
Logging startup info including buffer and batch sizes is beneficial.
147-149: Authorization string built from config.BasicAuth.
Straightforward and easy to maintain.
156-158: Ticker-based flush logic.
Schedules flush usingconfig.FlushInterval; well-aligned with the earlierLoadConfigdefaults.
159-164: Defining flushAndReset closure.
Encapsulating flush logic aids readability and maintainability.
166-167: Flush metrics recording.
Captures flush reason and increments the flush counter. Good metric design.
170-180: Sample logging for ticker flush.
This approach prevents log spam by logging every 10th flush.
182-188: Logging flush stats when sampling.
Clearly logs reason and row count.
191-192: Invoking persist for each batch.
Integrating the context and config maintains consistency with the rest of the pipeline.
193-197: Error logging on flush.
Captures batch query for context, which helps debugging.
200-205: Tracking flush duration.
Recording the histogram metric is solid for performance insights.
207-209: Buffer and ticker reset.
Properly resets counters and reinitializes the ticker.
213-215: Flush on shutdown.
Great that you drain and persist everything prior to returning. Ensures no data loss.
229-229: No explicit change detected.
No line labeled with '~'. Presumably unmodified or not part of the diff.apps/chproxy/go.mod (3)
3-3: Bump Go Version to 1.24.0
The Go version has been updated from a previous version to 1.24.0. Please confirm that all build environments, CI/CD pipelines, and team members’ setups are aligned with this version to avoid compatibility issues.
5-16: Add Direct OpenTelemetry Dependencies
This section introduces a set of OpenTelemetry packages for bridging logs (otelslog), tracing, metrics, and related SDK components. The explicit versioning is good practice to ensure reproducibility. Verify that these versions are compatible with your application architecture and that integration tests confirm that telemetry data is correctly captured and exported.
18-35: Review Indirect Dependencies Block
A second require block now lists several indirect dependencies, including additional OpenTelemetry components and supporting libraries. Ensure that these indirect dependencies do not conflict with the direct ones and are properly managed via your dependency management workflow (e.g., usinggo mod tidy). It may be useful to periodically review these indirect dependencies for potential updates or conflicts, ensuring the overall dependency graph remains stable.
imeyer
force-pushed
the
chproxy-add-otel-instrumentation
branch
from
1cee266 to
9a7c72c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/chproxy/otel.go (1)
51-174: FunctionsetupTelemetryis large and could benefit from modularization.
While the logic is valid, consider splitting the OTLP exporters (logs, metrics, trace) initialization into smaller helper functions. This will improve clarity and testability.apps/chproxy/main.go (2)
36-113:persistfunction successfully integrates telemetry metrics and tracing spans.
The per-error metric increments, trace recording, and final status assignments are a well-structured approach to capturing issues. Consider whether you need partial retry or a circuit breaker if ClickHouse becomes unavailable for extended periods.
115-356: Naming mismatch in ticker-based flush log message.
At lines 242-245, the debug log says"flushing due to max batch size"even though the flush reason is"ticker". This can cause confusion when reading logs.-case <-ticker.C: - config.Logger.Debug("flushing due to max batch size", +case <-ticker.C: + config.Logger.Debug("flushing due to ticker interval", "buffered_rows", buffered, "max_size", config.MaxBatchSize) flushAndReset(ctx, "ticker")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/chproxy/go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
apps/chproxy/config.go(1 hunks)apps/chproxy/go.mod(1 hunks)apps/chproxy/main.go(5 hunks)apps/chproxy/otel.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/chproxy/config.go
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: autofix
- GitHub Check: Test Go API Local / test_agent_local
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
apps/chproxy/otel.go (2)
1-21: Overall import structure looks clean.
No major concerns here, and all imported OTEL packages appear relevant to telemetry functionality.
23-37: Struct definition appears comprehensive.
Storing references to the meter, tracer, and log handler in a single config object centralizes telemetry resources effectively.apps/chproxy/main.go (1)
8-9: Confirm usage of both standard librarylogandlog/slog.
Since you’re defaulting toslog.SetDefault(config.Logger), ensure you intend to keep the plainlogpackage for fallback or debug logs. Otherwise, it might be redundant.apps/chproxy/go.mod (3)
3-3: Ensure environment supports Go 1.24.0.
Confirm your CI/CD environment and production runtime are ready for this version, as it might introduce new language features or runtime behaviors.
5-16: New dependencies for OTEL instrumentation look valid.
All pinned versions appear explicitly aligned with the OTEL modules inotel.go.
18-35: Indirect dependencies.
These additions are typical for advanced telemetry usage. No immediate concerns detected.
Metrics, Tracing, and Logs oh my! Signed-off-by: Ian Meyer (imeyer) <k@imeyer.io>
imeyer
force-pushed
the
chproxy-add-otel-instrumentation
branch
from
9a7c72c to
4e581b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/chproxy/otel.go (1)
51-174: Comprehensive OTEL setup with robust error checks.
Partial cleanup on errors
Currently, if the metric provider or other OTEL components fail to initialize, the function returns immediately, and any previously initialized parts may not be shut down. Consider a more robust approach if partial setups succeed but subsequent ones fail.Graceful shutdown
The cleanup function is thorough. However, you might unify errors from each shutdown call into a single aggregated error instead of returning on the first failure, to ensure all resources attempt to shut down.Optional test coverage
Testing each major code path (especially error conditions) can help confirm that telemetry setup and shutdown work properly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/chproxy/go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
apps/chproxy/config.go(1 hunks)apps/chproxy/go.mod(1 hunks)apps/chproxy/main.go(5 hunks)apps/chproxy/otel.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/chproxy/config.go
- apps/chproxy/main.go
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
apps/chproxy/otel.go (3)
1-21: Looks good importing OTEL dependencies.No issues observed in these import statements or package declarations.
23-37: TelemetryConfig struct is well-organized.The struct fields are descriptive, and the usage of variables for separate metric and tracing configuration is clear.
39-49: Potential concurrency data race oncurrentBufferSize.This issue was raised previously. Calling
SetBufferSizeandGetBufferSizeconcurrently without synchronization can cause race conditions. Consider using atomic operations or a mutex to protect access tocurrentBufferSize.apps/chproxy/go.mod (2)
18-35: No immediate concerns about indirect dependencies.These dependencies appear to be standard transitive requirements for OTEL.
5-16:Details
❓ Verification inconclusive
Confirm versions and security for introduced OTEL packages.
It's prudent to verify these pinned versions to ensure they contain the latest fixes and no known vulnerabilities.
🏁 Script executed:
#!/bin/bash # Using 'go list -m' to display module versions and any known vulnerabilities go list -m -u all | grep -E 'go.opentelemetry.io/(contrib|otel)'Length of output: 180
Below is an updated verification instruction. It appears the original command was run from a directory without a go.mod file, which caused the error. Please re-run the check from the module’s directory so we can properly confirm the OTEL package versions and their security status.
Verify OTEL module versions from the correct directory
- Make sure you’re in the module directory (e.g.
apps/chproxy).- Then run the following command:
#!/bin/bash cd apps/chproxy && go list -m -u all | grep -E 'go.opentelemetry.io/(contrib|otel)'This adjusted command should output the relevant module versions along with any available update information. Once you’ve confirmed that the versions are current and free from known vulnerabilities, please update the review accordingly.
| module github.com/unkeyed/unkey/apps/chproxy | ||
|
|
||
| go 1.23.2 | ||
| go 1.24.0 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Validate your environment for Go 1.24.0.
Ensure your build environment is set up with Go 1.24.0 or greater.
🏁 Script executed:
#!/bin/bash
# Check installed Go version
go versionLength of output: 44
Action Required: Update your Go installation to version 1.24.0 or later.
- The
apps/chproxy/go.modfile specifiesgo 1.24.0, but the environment is currently running Go 1.23.3. - This version mismatch can lead to unexpected build issues. Please update your Go environment accordingly.
3 participants
Metrics, Tracing, and Logs oh my!
Summary by CodeRabbit
New Features
Refactor
Chores