Merged
Conversation
- telemetry/ → observability/ (with log/ merged as observability/logger/) - jsondiff/, rql/ → data/jsondiff/, data/rql/ - Fix otelhhtpclient typo → otelhttpclient - Inline utils/error_status into observability/otelgrpc/status.go, delete utils/ - Rename log package to logger to match directory - Update all internal imports and README
📝 WalkthroughWalkthroughThis PR reorganizes and relocates multiple packages: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
data/jsondiff/README.md (1)
26-32:⚠️ Potential issue | 🟡 MinorRemove unused
stringsimport from both Go examples.
As written, these snippets include an unused import and won’t compile when copied as-is.✂️ Proposed doc fix
import ( "encoding/json" "fmt" "reflect" - "strings" "github.com/raystack/salt/data/jsondiff" )Also applies to: 103-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/jsondiff/README.md` around lines 26 - 32, The Go examples import the "strings" package but never use it, causing compilation errors; remove the unused "strings" import from both import blocks in the README examples (the import lists that currently include "encoding/json", "fmt", "reflect", "strings", "github.com/raystack/salt/data/jsondiff") so that only used packages remain (e.g., "encoding/json", "fmt", "reflect", "github.com/raystack/salt/data/jsondiff").
🧹 Nitpick comments (1)
observability/logger/logrus_test.go (1)
21-72: Consider extracting a shared test logger helper to remove duplication.The same
NewLogrus(...WithLevel...WithWriter...WithFormatter...)setup is repeated in every subtest.♻️ Suggested refactor
func TestLogrus(t *testing.T) { + newTestLogger := func(level string, w *bufio.Writer) logger.Logger { + return logger.NewLogrus( + logger.LogrusWithLevel(level), + logger.LogrusWithWriter(w), + logger.LogrusWithFormatter(&logrus.TextFormatter{DisableTimestamp: true}), + ) + } + t.Run("should parse info messages at debug level correctly", func(t *testing.T) { var b bytes.Buffer foo := bufio.NewWriter(&b) - - logger := logger.NewLogrus(logger.LogrusWithLevel("debug"), logger.LogrusWithWriter(foo), logger.LogrusWithFormatter(&logrus.TextFormatter{ - DisableTimestamp: true, - })) + logger := newTestLogger("debug", foo) logger.Info("hello world") foo.Flush() @@ t.Run("should not parse debug messages at info level correctly", func(t *testing.T) { var b bytes.Buffer foo := bufio.NewWriter(&b) - - logger := logger.NewLogrus(logger.LogrusWithLevel("info"), logger.LogrusWithWriter(foo), logger.LogrusWithFormatter(&logrus.TextFormatter{ - DisableTimestamp: true, - })) + logger := newTestLogger("info", foo)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@observability/logger/logrus_test.go` around lines 21 - 72, Extract a small test helper (e.g., newTestLogger or setupTestLogger) that returns the created logger and its underlying bufio.Writer/bytes.Buffer pair to eliminate repeated setup calls; move the repeated NewLogrus(logger.LogrusWithLevel(...), logger.LogrusWithWriter(...), logger.LogrusWithFormatter(...)) logic into that helper and update each subtest to call the helper and use the returned logger and writer before calling foo.Flush() and reading buffer contents. Ensure the helper uses the same parameters (DisableTimestamp: true) and exposes the buffer for assertions so tests like those invoking NewLogrus, LogrusWithLevel, LogrusWithWriter, and LogrusWithFormatter remain unchanged except for using the helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@observability/otelgrpc/otelgrpc.go`:
- Line 32: The struct tag on meterOptions.meterName isn't applied automatically,
so update NewMeter to use the intended default when meterOpts.meterName is
empty: detect when meterOptions.meterName == "" inside NewMeter and set it to
"github.com/raystack/salt/observability/otelgrpc" before calling otel.Meter;
alternatively initialize meterOptions with that default in the meterOptions
creation path so otel.Meter never receives an empty string. Ensure references
are to meterOptions.meterName and the NewMeter function.
---
Outside diff comments:
In `@data/jsondiff/README.md`:
- Around line 26-32: The Go examples import the "strings" package but never use
it, causing compilation errors; remove the unused "strings" import from both
import blocks in the README examples (the import lists that currently include
"encoding/json", "fmt", "reflect", "strings",
"github.com/raystack/salt/data/jsondiff") so that only used packages remain
(e.g., "encoding/json", "fmt", "reflect",
"github.com/raystack/salt/data/jsondiff").
---
Nitpick comments:
In `@observability/logger/logrus_test.go`:
- Around line 21-72: Extract a small test helper (e.g., newTestLogger or
setupTestLogger) that returns the created logger and its underlying
bufio.Writer/bytes.Buffer pair to eliminate repeated setup calls; move the
repeated NewLogrus(logger.LogrusWithLevel(...), logger.LogrusWithWriter(...),
logger.LogrusWithFormatter(...)) logic into that helper and update each subtest
to call the helper and use the returned logger and writer before calling
foo.Flush() and reading buffer contents. Ensure the helper uses the same
parameters (DisableTimestamp: true) and exposes the buffer for assertions so
tests like those invoking NewLogrus, LogrusWithLevel, LogrusWithWriter, and
LogrusWithFormatter remain unchanged except for using the helper.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e466f661-55be-4c13-aac3-b42d77fd8993
📒 Files selected for processing (28)
README.mdauth/audit/repositories/dockertest_test.goauth/audit/repositories/postgres_test.godata/jsondiff/README.mddata/jsondiff/jsondiff.godata/jsondiff/jsondiff_test.godata/jsondiff/jsondiff_wi2l.godata/jsondiff/jsondiff_wi2l_test.godata/jsondiff/reconstructor.godata/rql/README.mddata/rql/parser.godata/rql/parser_test.goobservability/logger/logger.goobservability/logger/logrus.goobservability/logger/logrus_test.goobservability/logger/noop.goobservability/logger/zap.goobservability/logger/zap_test.goobservability/opentelemetry.goobservability/otelgrpc/otelgrpc.goobservability/otelgrpc/otelgrpc_test.goobservability/otelgrpc/status.goobservability/otelgrpc/status_test.goobservability/otelhttpclient/annotations.goobservability/otelhttpclient/http_transport.goobservability/otelhttpclient/http_transport_test.goobservability/telemetry.gotesting/dockertestx/postgres.go
| } | ||
| type MeterOpts struct { | ||
| meterName string `default:"github.com/raystack/salt/telemetry/otelgrpc"` | ||
| meterName string `default:"github.com/raystack/salt/observability/otelgrpc"` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where NewMeter is used and whether WithMeterName is passed at call sites.
rg -nP --type=go '\bNewMeter\s*\('
rg -nP --type=go '\bWithMeterName\s*\('Repository: raystack/salt
Length of output: 233
🏁 Script executed:
# First, let's read the otelgrpc.go file to understand the actual implementation
cat -n observability/otelgrpc/otelgrpc.goRepository: raystack/salt
Length of output: 6255
🏁 Script executed:
# Find all call sites of NewMeter across the repository
rg -A2 '\bNewMeter\(' --type=goRepository: raystack/salt
Length of output: 271
🏁 Script executed:
# Check if there's any struct tag processing (reflect, struct tag parsing) in the file
rg -n 'reflect|struct|tag|default' observability/otelgrpc/otelgrpc.go --type=goRepository: raystack/salt
Length of output: 216
🏁 Script executed:
# Search for all NewMeter call sites across the entire repository
rg 'NewMeter' --type=go -B2 -A2Repository: raystack/salt
Length of output: 755
🏁 Script executed:
# Check if there's any struct tag processing library imported or used
rg 'reflect|defaults|struct' observability/otelgrpc/otelgrpc.go -A2 -B2Repository: raystack/salt
Length of output: 373
🏁 Script executed:
# Find test files related to otelgrpc
fd -e test.go -e _test.go observabilityRepository: raystack/salt
Length of output: 39
🏁 Script executed:
# Check all imports in otelgrpc.go to confirm no struct tag processing library
head -20 observability/otelgrpc/otelgrpc.goRepository: raystack/salt
Length of output: 429
🏁 Script executed:
# One final check: search for any usage of WithMeterName in tests or examples
rg 'WithMeterName' --type=go -B3 -A3Repository: raystack/salt
Length of output: 443
Default meter name is not actually applied in NewMeter.
Line 32's struct tag default does not initialize meterOpts.meterName; NewMeter still starts with an empty value on Line 42, so otel.Meter(...) on Line 46 receives "" unless an option is passed. Go does not automatically process struct tag defaults—they are pure metadata.
Proposed fix
type MeterOpts struct {
- meterName string `default:"github.com/raystack/salt/observability/otelgrpc"`
+ meterName string
}
+
+const defaultMeterName = "github.com/raystack/salt/observability/otelgrpc"
func NewMeter(hostName string, opts ...Option) Meter {
- meterOpts := &MeterOpts{}
+ meterOpts := &MeterOpts{meterName: defaultMeterName}
for _, opt := range opts {
opt(meterOpts)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@observability/otelgrpc/otelgrpc.go` at line 32, The struct tag on
meterOptions.meterName isn't applied automatically, so update NewMeter to use
the intended default when meterOpts.meterName is empty: detect when
meterOptions.meterName == "" inside NewMeter and set it to
"github.com/raystack/salt/observability/otelgrpc" before calling otel.Meter;
alternatively initialize meterOptions with that default in the meterOptions
creation path so otel.Meter never receives an empty string. Ensure references
are to meterOptions.meterName and the NewMeter function.
whoAbhishekSah
approved these changes
Mar 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
telemetry/→observability/: Mergedlog/intoobservability/logger/, moved otel sub-packages alongsidejsondiff/,rql/→data/: Grouped data processing utilities under a common parentutils/deleted: Inlined gRPC status helpers intoobservability/otelgrpc/status.gootelhhtpclienttypo fixed →otelhttpclientTest plan
go build ./...compiles cleanlySummary by CodeRabbit
Documentation
Refactor
observability,data, andclinamespaces for improved code organization.