Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Contributor
📝 WalkthroughWalkthroughReplaces the old OTEL logging package with a new internal Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Middleware
participant Handler
participant LoggerPkg
participant Sampler
Client->>Middleware: HTTP request
Middleware->>LoggerPkg: StartWideEvent(ctx, "request", attrs...)
Middleware->>Handler: invoke handler(ctx)
alt handler records attrs or error
Handler->>LoggerPkg: Set(ctx, attrs...) / SetError(ctx, err)
end
Middleware->>LoggerPkg: event.End()
LoggerPkg->>Sampler: Sample(event)
alt sampler allows emission
LoggerPkg->>LoggerPkg: Format entry (Info/Error)
LoggerPkg->>LoggerPkg: Emit via handlers
else drop
LoggerPkg-->>LoggerPkg: Discard event
end
Middleware->>Client: HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
chronark
commented
Feb 5, 2026
chronark
requested review from
Flo4604,
MichaelUnkey,
imeyer,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
pkg/prometheus/server.go (1)
35-47:⚠️ Potential issue | 🟡 MinorUpdate docs: remove Config param + fix example.
Comment still references
configand oldprometheus.Config{}usage. Update toNew().Suggested diff
-// Parameters: -// - config: Configuration for the server, including required dependencies. -// -// Returns: +// Returns: // - A configured zen.Server ready to be started. // - An error if server creation fails, typically due to invalid configuration. // // Example usage: // // // Create a dedicated metrics server -// server, err := prometheus.New(prometheus.Config{ -// , -// }) +// server, err := prometheus.New()pkg/runner/runner_test.go (1)
11-20:⚠️ Potential issue | 🟡 MinorUse t.Run subtests for scenarios.
As per coding guidelines: "/*_test.go: Organize tests usingt.Run()for scenarios."Also applies to: 22-43, 45-66, 68-89, 91-113
pkg/circuitbreaker/lib_test.go (1)
15-92:⚠️ Potential issue | 🟡 MinorUse t.Run subtests for scenarios.
As per coding guidelines: "/*_test.go: Organize tests usingt.Run()for scenarios."pkg/zen/server.go (1)
78-86:⚠️ Potential issue | 🟡 MinorMalformed comment in example code.
Line 81 appears to have a leftover comma from removing the Logger field in the example.
Proposed fix
// Example: // // server, err := zen.New(zen.Config{ // InstanceID: "api-server-1", -// , // })
🤖 Fix all issues with AI agents
In `@cmd/vault/main.go`:
- Around line 60-64: Add validation in each service Config.Validate() to enforce
log sampler constraints: call assert.InRange(cfg.LogSampleRate, 0.0, 1.0,
"LogSampleRate") to ensure LogSampleRate ∈ [0,1] and call
assert.Greater(cfg.LogSlowThreshold, 0, "LogSlowThreshold") to ensure
LogSlowThreshold > 0; update svc/vault/config.go (and the same checks in
svc/frontline/config.go, svc/api/config.go, svc/krane/config.go,
svc/sentinel/config.go, svc/preflight/config.go) inside the Config.Validate()
method so the function returns a validation error when those assertions fail.
In `@internal/services/auditlogs/service.go`:
- Around line 10-13: The package comment above the type service is out of date —
it mentions a logger dependency that was removed; update the doc comment for the
service struct (type service) and the surrounding top-of-file comments to
accurately describe current responsibilities (database persistence and
transactional batch insert only) and remove any reference to structured logger
or logger injection; ensure comments around the service struct and any related
functions (constructor/newService, methods on service) reflect current deps and
behavior.
In `@pkg/clickhouse/flush.go`:
- Around line 37-40: The logger.Error call inside the deferred function should
pass the error value instead of converting it to a string; update the defer in
pkg/clickhouse/flush.go so the call uses logger.Error("failed to close batch",
"error", err) (referencing batch.Close and the deferred anonymous func) to
preserve the error type for structured logging via slog.Logger.
In `@pkg/logger/event.go`:
- Around line 89-104: Replace the []any payload with a typed []slog.Attr:
construct attrs := []slog.Attr{ slog.GroupAttrs("errors", errors...),
slog.GroupAttrs("log_meta", slog.Time("start", e.start),
slog.Duration("duration", time.Since(e.start))), } then append e.attrs into
attrs, and replace the logger.Error/logger.Info calls with logger.LogAttrs using
the proper level (slog.LevelError for the error branch, slog.LevelInfo for the
non-error branch) and pass attrs... (refer to logger, e.attrs, e.errors,
e.message and e.start in pkg/logger/event.go).
In `@pkg/logger/global.go`:
- Around line 52-56: AddBaseAttrs currently appends to baseAttrs but those
attributes are never used; update all logging emission sites (the simple
functions Debug, Info, Warn, Error and the Event.End method) to include
baseAttrs when they call the underlying slog logger or emit events, or
alternatively remove AddBaseAttrs and baseAttrs if you prefer to drop the
feature. Concretely, locate where Debug/Info/Warn/Error call the logger and
modify those calls to merge/apply baseAttrs with the per-call attrs before
logging, and do the same in Event.End so baseAttrs are appended to every emitted
log entry (refer to symbols AddBaseAttrs, baseAttrs, Debug, Info, Warn, Error,
Event.End). Ensure mu protects reads of baseAttrs when merging so concurrent
access remains safe.
In `@pkg/logger/wide.go`:
- Around line 18-22: The doc example incorrectly references a non-existent
package-level End function; update the example for StartWideEvent to capture the
returned *Event and defer its End() method instead of calling logger.End.
Specifically, change the example invocation to assign both return values from
StartWideEvent (e.g., ctx, ev := StartWideEvent(...)) and use defer ev.End() so
Event.End is called; ensure the names StartWideEvent and Event.End are used to
locate the code to modify.
🧹 Nitpick comments (10)
svc/api/integration/cluster/cache/e2e_test.go (1)
49-50: Use timing.HeaderName constant. Keeps header name single-source and consistent with later usage.Diff
- timingHeaders := resp.Headers.Values("X-Unkey-Timing") + timingHeaders := resp.Headers.Values(timing.HeaderName)pkg/vault/keyring/roll_keys.go (1)
34-36: Consider passingctxto logger for trace correlation.Tracing span is active but
logger.Infodoesn't receive context—logs may not correlate with traces. If the new logger package supports context, consider using it.pkg/vault/storage/s3.go (1)
32-32: Consider adding structured fields for observability.These init-time logs could include the bucket name and endpoint for easier debugging in multi-tenant or multi-bucket deployments.
Example with structured fields
- logger.Info("using s3 storage") + logger.Info("using s3 storage", "bucket", config.S3Bucket, "url", config.S3URL)- logger.Info("creating bucket if necessary") + logger.Info("creating bucket if necessary", "bucket", config.S3Bucket)- logger.Info("s3 storage initialized") + logger.Info("s3 storage initialized", "bucket", config.S3Bucket)Also applies to: 55-55, 63-63
pkg/zen/README.md (1)
139-141: Consider documenting the logger API conventions.The examples show two different patterns for logging:
- Line 139-141:
logger.Info("message", "key", "value")- Line 151:
logger.Error("message", slog.String("key", value))Consider adding a note about when to use plain key-value pairs versus
slog.*attribute constructors for consistency. This will help users understand the logger package conventions.Also applies to: 151-151
pkg/cache/cache.go (1)
556-560: Consider adding key context to the log statement.Unlike
revalidate(line 302) andrevalidateMany(line 605), this log doesn't include the key being revalidated, which could make debugging harder.♻️ Suggested improvement
if err != nil && !db.IsNotFound(err) { - logger.Warn("failed to revalidate with canonical key", "error", err.Error()) + logger.Warn("failed to revalidate with canonical key", "error", err.Error(), "key", canonicalKey) return }svc/api/config.go (1)
128-134: Consider validatingLogSampleRatebounds.
LogSampleRateis documented as 0.0-1.0 butValidate()doesn't enforce this. Values outside this range could cause unexpected sampling behavior.Proposed validation
func (c Config) Validate() error { + if c.LogSampleRate < 0 || c.LogSampleRate > 1 { + return fmt.Errorf("LogSampleRate must be between 0.0 and 1.0, got %f", c.LogSampleRate) + } // TLS configuration is validated when it's created from filespkg/zen/redact_test.go (1)
227-248: Fuzz test looks good for crash detection.The nil check condition on line 240 is unnecessary since
redactreturns the input unchanged for non-matching content (never nil for valid[]byte). Consider simplifying:- if len(input) > 0 { - require.NotNil(t, result) - } + require.NotNil(t, result)pkg/logger/sampler.go (1)
34-43: Consider adding validation forSampleRatebounds.
SampleRateis documented as "probabilities between 0.0 (never) and 1.0 (always)" but there's no validation. Values outside this range (e.g., negative or >1) would cause unexpected behavior: negative rates never sample, rates >1 always sample.♻️ Optional: Add bounds check in Sample
func (s TailSampler) Sample(event *Event) bool { - rate := rand.Float64() + r := rand.Float64() if len(event.errors) > 0 { return true } if event.duration > s.SlowThreshold { return true } - if rate < s.SampleRate { + if r < s.SampleRate { return true } return false }pkg/logger/global.go (1)
17-22: Redundant mutex initialization in init().
sync.Mutex{}zero value is ready to use; explicit assignment is unnecessary.♻️ Simplify init
func init() { - mu = sync.Mutex{} logger = slog.Default() sampler = AlwaysSample{} baseAttrs = []slog.Attr{} }pkg/cache/clustering/e2e_test.go (1)
19-121: Wrap scenario in t.Run().Patch
func TestClusterCache_EndToEndDistributedInvalidation(t *testing.T) { + t.Run("distributed invalidation", func(t *testing.T) { brokers := containers.Kafka(t) @@ _, hit2After := localCache2.Get(ctx, "shared-key") require.Equal(t, cache.Miss, hit2After, "Node 2's cache should be invalidated after receiving event from Node 1") + }) }As per coding guidelines:
**/*_test.go: Organize tests usingt.Run()for scenarios.
Comment on lines
10
to
13
| // service implements the AuditLogService interface, providing audit logging | ||
| // functionality with database persistence and structured logging capabilities. | ||
| // The service handles batch insertion of audit logs within transactional contexts. | ||
| type service struct { |
Contributor
There was a problem hiding this comment.
Docs mention logger but logger removed.
Update comments to match current deps.
Diff
-// functionality with database persistence and structured logging capabilities.
+// functionality with database persistence.
// The service handles batch insertion of audit logs within transactional contexts.
-// Config contains the dependencies required to initialize an audit log service.
-// Both fields are required for proper service operation.
+// Config contains dependencies required to initialize an audit log service.
type Config struct {
DB db.Database // Database interface for audit log persistence
}
// New creates a new audit log service instance with the provided configuration.
// The returned service implements AuditLogService and is ready for immediate use.
-// Both database and logger dependencies must be provided in the config.
+// Database dependency must be provided in the config.
func New(cfg Config) (*service, error) {Also applies to: 19-28
🤖 Prompt for AI Agents
In `@internal/services/auditlogs/service.go` around lines 10 - 13, The package
comment above the type service is out of date — it mentions a logger dependency
that was removed; update the doc comment for the service struct (type service)
and the surrounding top-of-file comments to accurately describe current
responsibilities (database persistence and transactional batch insert only) and
remove any reference to structured logger or logger injection; ensure comments
around the service struct and any related functions (constructor/newService,
methods on service) reflect current deps and behavior.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
svc/api/routes/v2_apis_list_keys/handler.go (1)
245-250:⚠️ Potential issue | 🟡 MinorSilent decryption failures may confuse callers.
When decryption fails, the key is included in the response without plaintext. Caller has no way to distinguish "key has no encrypted value" from "decryption failed". Consider either:
- Adding a
decryptionFailedfield to the response- Returning a partial error/warning in the response metadata
svc/api/routes/v2_keys_get_key/412_test.go (1)
48-48:⚠️ Potential issue | 🟡 MinorStale comment doesn't match test name.
Comment says "Test case for API ID with special characters" but test is "Try getting a recoverable key without being opt-in".
Suggested fix
- // Test case for API ID with special characters + // Test case for requesting decryption without opt-in t.Run("Try getting a recoverable key without being opt-in", func(t *testing.T) {pkg/zen/session_body_limit_test.go (2)
193-211:⚠️ Potential issue | 🟠 MajorAvoid
any; use typed structGuideline says no
any. Use a typed response struct, keep checks.Minimal diff
- var response map[string]any - err = json.Unmarshal(w.Body.Bytes(), &response) - require.NoError(t, err, "Response should be valid JSON") - - // Validate that response contains error field - require.Contains(t, response, "error", "Response should contain 'error' field") - - errorObj, ok := response["error"].(map[string]any) - require.True(t, ok, "Error field should be an object") - - // Validate required JSON fields in error object - require.Contains(t, errorObj, "title", "Error should contain 'title' field") - require.Contains(t, errorObj, "detail", "Error should contain 'detail' field") - require.Contains(t, errorObj, "status", "Error should contain 'status' field") - - // Validate field values - require.Equal(t, "Request Entity Too Large", errorObj["title"]) - require.Equal(t, float64(413), errorObj["status"]) // JSON unmarshals numbers as float64 - require.Contains(t, errorObj["detail"], "request body exceeds") + type errorResp struct { + Error struct { + Title string `json:"title"` + Detail string `json:"detail"` + Status int `json:"status"` + } `json:"error"` + } + var response errorResp + err = json.Unmarshal(w.Body.Bytes(), &response) + require.NoError(t, err, "Response should be valid JSON") + + require.Equal(t, "Request Entity Too Large", response.Error.Title) + require.Equal(t, 413, response.Error.Status) + require.Contains(t, response.Error.Detail, "request body exceeds")
74-96:⚠️ Potential issue | 🟡 MinorWrap single-scenario tests in
t.RunGuideline: *_test.go uses
t.Runfor scenarios. Apply to these tests.Example pattern (apply to each)
func TestSession_BodySizeLimitWithBindBody(t *testing.T) { - // Test that BindBody still works correctly with body size limits - bodyContent := `{"name":"test","value":42}` - - req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(bodyContent)) - req.Header.Set("Content-Type", "application/json") - w := httptest.NewRecorder() - - sess := &Session{} - err := sess.Init(w, req, 1024) // 1KB limit - require.NoError(t, err) - - type TestData struct { - Name string `json:"name"` - Value int `json:"value"` - } - - var data TestData - err = sess.BindBody(&data) - require.NoError(t, err) - require.Equal(t, "test", data.Name) - require.Equal(t, 42, data.Value) + t.Run("bind body with limit", func(t *testing.T) { + bodyContent := `{"name":"test","value":42}` + + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(bodyContent)) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + + sess := &Session{} + err := sess.Init(w, req, 1024) // 1KB limit + require.NoError(t, err) + + type TestData struct { + Name string `json:"name"` + Value int `json:"value"` + } + + var data TestData + err = sess.BindBody(&data) + require.NoError(t, err) + require.Equal(t, "test", data.Name) + require.Equal(t, 42, data.Value) + }) }Also applies to: 153-215, 217-239
pkg/prometheus/server.go (1)
35-47:⚠️ Potential issue | 🟡 MinorUpdate New() docs/example for no-arg signature.
Doc still mentions Config param; example has dangling comma.✏️ Proposed doc fix
-// Parameters: -// - config: Configuration for the server, including required dependencies. -// -// Returns: -// - A configured zen.Server ready to be started. -// - An error if server creation fails, typically due to invalid configuration. +// Returns: +// - A configured zen.Server ready to be started. +// - An error if server creation fails. @@ -// server, err := prometheus.New(prometheus.Config{ -// , -// }) +// server, err := prometheus.New()internal/services/auditlogs/service.go (1)
19-28:⚠️ Potential issue | 🟡 MinorUpdate doc comments; logger mention stale. Remove “both fields/logger” wording.
Proposed edit
-// Both fields are required for proper service operation. +// DB required for proper service operation. @@ -// Both database and logger dependencies must be provided in the config. +// Database dependency must be provided in the config.
🤖 Fix all issues with AI agents
In `@cmd/api/main.go`:
- Around line 115-119: In svc/api/config.go inside the Validate() method, add
assertions for the logging fields so invalid values can’t be accepted: use
assert.InRange to ensure the LogSampleRate (or log-sample-rate field) is between
0 and 1 inclusive, and use an appropriate assert helper (e.g.,
assert.GreaterOrEqual or assert.InRange with min 0) to ensure LogSlowThreshold
(or log-slow-threshold field) is >= 0; update any error messages to reference
the field names and keep checks applied in the same Validate() path that already
validates other config fields.
In `@cmd/frontline/main.go`:
- Around line 5-6: Add range checks in Config.Validate(): ensure the
LogSampleRate field is between 0 and 1 inclusive (0.0 <= LogSampleRate <= 1.0)
and that LogSlowThreshold is non‑negative (LogSlowThreshold >= 0). Inside the
Config.Validate() method, return a descriptive error when either check fails
(mentioning the invalid value and expected range) so invalid configs cannot be
constructed; reference the Config type and its Validate() method along with the
LogSampleRate and LogSlowThreshold fields when implementing the validation.
In `@cmd/preflight/main.go`:
- Around line 36-37: The flag defined with cli.Float("log-sample-rate", ...)
allows values outside 0.0–1.0; add a validation step after flag parsing (e.g.,
in main or the function that reads CLI values) that checks the parsed
log-sample-rate value and returns an error or exits with a clear message if
value < 0.0 or > 1.0. Locate where the CLI flags are parsed and the resulting
variable for "log-sample-rate" (the value produced by
cli.Float("log-sample-rate", ...)), validate using a simple conditional (if rate
< 0.0 || rate > 1.0) and fail fast with fmt.Errorf or a non-zero exit +
user-facing log explaining the allowed range.
In `@cmd/sentinel/main.go`:
- Around line 56-60: Validate the CLI flags for the logging sampler immediately
after parsing: ensure the "log-sample-rate" value (from the cli.Float flag) is
in [0.0, 1.0] and the "log-slow-threshold" value (from the cli.Duration flag) is
>= 0; if either check fails return a clear error and abort startup rather than
accepting the value. Move these validated values into a small typed config
(e.g., LogSamplerConfig or similar) used by the sampler creation code so illegal
states are unrepresentable, and reference the flag names "log-sample-rate" and
"log-slow-threshold" when constructing the config and emitting the validation
error.
In `@cmd/vault/main.go`:
- Around line 60-64: Add validation and domain types to prevent invalid sampler
values: introduce a domain type like type SampleRate float64 (and optionally
type NonNegativeDuration time.Duration) and use those types for
Config.LogSampleRate and Config.LogSlowThreshold; then update Config.Validate()
to return an error if LogSampleRate is <0 or >1 and if LogSlowThreshold is
negative (ensure comparisons use the new types), providing clear error messages.
Adjust any parsing/assignment code that constructs Config so it
converts/validates CLI-provided float64/duration into the domain types at parse
time to make invalid states unrepresentable before validation (references:
Config.Validate, Config.LogSampleRate, Config.LogSlowThreshold, SampleRate).
In `@pkg/logger/event.go`:
- Around line 89-104: The slice currently declared as casted := []any should be
changed to a typed slice of slog.Attr to preserve type safety; build casted as
[]slog.Attr and append the results of slog.GroupAttrs("errors", errors...),
slog.GroupAttrs("log_meta", slog.Time("start", e.start),
slog.Duration("duration", time.Since(e.start))), and then loop over e.attrs
appending each attr (they are already slog.Attr); ensure the same casted slice
is passed into logger.Error and logger.Info so the calls use []slog.Attr instead
of []any (refer to the variables casted, e.attrs, e.errors and the functions
slog.GroupAttrs, slog.Time, slog.Duration).
In `@pkg/logger/global.go`:
- Around line 64-67: SetSampler currently assigns the provided Sampler directly
which allows sampler to become nil and later panic in Event.End; update
SetSampler to guard against nil by locking mu, and if the incoming s is nil keep
the existing sampler (or set a safe default/no-op sampler) instead of assigning
nil; ensure the function references sampler and mu and that Event.End will never
observe a nil sampler.
In `@pkg/logger/slog.go`:
- Around line 3-29: The Debug/Info/Warn/Error wrapper functions use variadic
args ...any which erases type safety; change their signatures to accept attrs
...slog.Attr and inside each function call logger.LogAttrs(level, msg, attrs...)
(or logger.LogAttrs with the appropriate level wrapper if available) instead of
logger.Debug/Info/Warn/Error so callers can pass typed slog.Attr slices
(matching how event.go constructs attrs); update function signatures Debug,
Info, Warn, Error to use attrs ...slog.Attr and replace the internal calls to
use logger.LogAttrs with the message and attrs.
In `@pkg/zen/middleware_logger.go`:
- Line 39: The log currently uses s.r.URL.Host which is usually empty for
incoming server requests; change the logger to use s.r.Host instead (replace
references to s.r.URL.Host with s.r.Host) so middleware_logger.go's logging
(where s.r.URL.Host is used) captures the actual request host for server-side
requests; confirm any tests or other log fields referencing s.r.URL.Host are
updated accordingly.
In `@pkg/zen/redact_test.go`:
- Around line 18-61: Replace real-looking secret literals in the redact_test.go
fixtures with non-sensitive placeholders: locate the table-driven test entries
whose name strings include "simple key value is redacted", "key value with no
whitespace around colon", "key value with extra whitespace around colon", etc.,
and change values like "sk_live_abc123", "sk_live_abcdef123456", empties with a
safe placeholder such as "sk_live_PLACEHOLDER" or "REDACTED_PLACEHOLDER" (or an
obviously fake value) so tests still assert redaction behavior but do not
contain secret-like literals; apply the same replacement to the other test
entries referenced in the comment (the blocks around 83-105, 127-134, 208-212,
227-246).
In `@pkg/zen/server.go`:
- Around line 79-82: The example snippet for creating a server using zen.New is
malformed due to a dangling comma and missing config fields; update the
zen.New(zen.Config{...}) example so the zen.Config literal is valid — e.g.,
provide required fields or remove the stray comma and close the struct properly;
locate the example around the zen.New call and the zen.Config literal
(references: zen.New, zen.Config, InstanceID, server) and ensure the config
block is syntactically correct and compiles as a minimal valid example.
In `@svc/api/config.go`:
- Around line 128-134: Add validation for the new LogSampleRate and
LogSlowThreshold fields inside the Config validation routine (e.g., Validate or
(*Config).Validate): ensure LogSampleRate is between 0.0 and 1.0 inclusive and
that LogSlowThreshold is non-negative; return a descriptive error mentioning the
offending field (LogSampleRate or LogSlowThreshold) when a check fails. Ensure
the checks run alongside other config validations so invalid values are rejected
early.
In `@svc/api/routes/v2_keys_get_key/412_test.go`:
- Around line 19-23: The handler initialization for route (the struct literal
assigned to route in function creating &handler.Handler with DB, Keys, Vault) is
missing the Auditlogs field present in other initializations (see where
Auditlogs: h.Auditlogs is used); either add Auditlogs: h.Auditlogs to this
struct literal for consistency or, if omission is intentional for test
isolation, add a brief comment explaining why Auditlogs is omitted so future
readers know it's deliberate.
🧹 Nitpick comments (8)
svc/api/integration/cluster/cache/e2e_test.go (1)
49-51: Usetiming.HeaderNameconstant for consistency.Hardcoded
"X-Unkey-Timing"here while lines 100 and 154 usetiming.HeaderName. Use the constant to avoid drift.Proposed fix
- timingHeaders := resp.Headers.Values("X-Unkey-Timing") + timingHeaders := resp.Headers.Values(timing.HeaderName)pkg/vault/integration/reusing_deks_test.go (1)
19-95: Use t.Run for test scenarios.Wrap each test body in a t.Run to align with test guideline.
As per coding guidelines "Organize tests usingt.Run()for scenarios".🛠️ Minimal refactor
func TestReuseDEKsForSameKeyring(t *testing.T) { + t.Run("same keyring reuses dek", func(t *testing.T) { s3 := containers.S3(t) @@ require.Len(t, deks, 1) + }) } @@ func TestIndividualDEKsPerKeyring(t *testing.T) { + t.Run("different keyrings get distinct deks", func(t *testing.T) { s3 := containers.S3(t) @@ require.Len(t, deks, 10) + }) }pkg/circuitbreaker/lib_test.go (1)
20-42: Optional: Uset.Run()for test phases.Per coding guidelines, tests should organize scenarios with
t.Run(). Also, lines 68-70 uset.Errorfinstead ofrequire.NoErrorfor consistency.♻️ Example refactor for TestCircuitBreakerStates
func TestCircuitBreakerStates(t *testing.T) { c := clock.NewTestClock() cb := New[int]("test", WithCyclicPeriod(5*time.Second), WithClock(c), WithTripThreshold(3)) - // Test Closed State - for i := 0; i < 3; i++ { - _, err := cb.Do(context.Background(), func(ctx context.Context) (int, error) { - return 0, errTestDownstream - }) - require.ErrorIs(t, err, errTestDownstream) - } - require.Equal(t, Open, cb.state) + t.Run("transitions to Open after threshold failures", func(t *testing.T) { + for i := 0; i < 3; i++ { + _, err := cb.Do(context.Background(), func(ctx context.Context) (int, error) { + return 0, errTestDownstream + }) + require.ErrorIs(t, err, errTestDownstream) + } + require.Equal(t, Open, cb.state) + })pkg/counter/interface.go (1)
154-157: Documentation example has unused parameter.The example signature
LoggingMiddleware(logger Logger)still accepts a logger parameter, but it's no longer used in the returned struct. Consider updating the signature to match:-// func LoggingMiddleware(logger Logger) Middleware { +// func LoggingMiddleware() Middleware { // return func(next Counter) Counter { // return &loggingCounter{next: next} // } // }pkg/eventstream/topic.go (1)
236-236: Minor inconsistency:errvserr.Error()in log calls.This file passes
errdirectly whileconsumer.go/producer.gouseerr.Error(). Passingerrdirectly is actually preferable (preserves error type), so the other files could be updated to match this pattern.Also applies to: 244-244
pkg/zen/redact_test.go (1)
10-11: Drop t.Parallel; redact not meant concurrent.
Based on learnings: “The redact function in go/pkg/zen/middleware_metrics.go is not expected to be called concurrently, so concurrency tests are unnecessary for this function.”Also applies to: 66-67, 74-75, 110-111, 118-119, 144-145, 152-153, 198-199, 206-207, 216-217
internal/services/ratelimit/replay.go (1)
42-42: Consider adding request context to the error log.Switching to global logger may lose instance-specific attributes previously attached to
s.logger. Consider adding identifiers to aid debugging:- logger.Error("failed to replay request", "error", err.Error()) + logger.Error("failed to replay request", + "error", err.Error(), + "name", req.Name, + "identifier", req.Identifier, + )pkg/zen/middleware_logger.go (1)
43-44: Consider body size limits for high-volume logging.Logging full request/response bodies (even redacted) on every request could increase log volume significantly. Consider truncating large bodies or making this configurable.
Flo4604
approved these changes
Feb 6, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/logger/doc.go`:
- Line 42: Fix the corrupted sentence in the package documentation in doc.go:
locate the package-level comment that begins "Tail sampling decides whether to
emit a log" and replace the malformed fragment "after t pgit puthe request
completes" with the correct phrase "after the request completes" (or equivalent
clear wording) so the doc reads e.g. "Tail sampling decides whether to emit a
log after the request completes." Ensure only the comment text is changed.
| // | ||
| // # Tail Sampling | ||
| // | ||
| // Tail sampling decides whether to emit a log after t pgit puthe request completes, rather |
Contributor
There was a problem hiding this comment.
Typo: corrupted text in documentation.
Line contains "after t pgit puthe request" - appears to be accidental insertion.
-// Tail sampling decides whether to emit a log after t pgit puthe request completes, rather
+// Tail sampling decides whether to emit a log after the request completes, rather📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Tail sampling decides whether to emit a log after t pgit puthe request completes, rather | |
| // Tail sampling decides whether to emit a log after the request completes, rather |
🤖 Prompt for AI Agents
In `@pkg/logger/doc.go` at line 42, Fix the corrupted sentence in the package
documentation in doc.go: locate the package-level comment that begins "Tail
sampling decides whether to emit a log" and replace the malformed fragment
"after t pgit puthe request completes" with the correct phrase "after the
request completes" (or equivalent clear wording) so the doc reads e.g. "Tail
sampling decides whether to emit a log after the request completes." Ensure only
the comment text is changed.
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.
Uh oh!
There was an error while loading. Please reload this page.