Conversation
Spground
requested review from
Generalwin,
Pangjiping,
fengcone,
hittyt,
kevinlynx and
ninan-nn
as code owners
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf761255be
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| start0 := time.Now() | ||
| err := r.Allocator.SyncSandboxAllocation(ctx, info.Sandbox, info.Pods) | ||
| latencyMs0 := time.Since(start0).Milliseconds() | ||
| allocatorSyncSingleAllocResultDurationSummary.WithLabelValues(info.Sandbox.Namespace, info.Sandbox.Spec.PoolRef, info.Sandbox.Name, strconv.FormatBool(err == nil)).Observe(float64(latencyMs0)) |
There was a problem hiding this comment.
This metric uses info.Sandbox.Name as a label value, which creates a distinct Summary time series for every sandbox ever seen. Since the controller never deletes label values, a cluster with normal sandbox churn will continuously grow in-memory metric state and scrape payload size over process lifetime, which can become a reliability issue in production. Keep this metric at bounded cardinality (e.g., namespace/pool/success only) and use logs for per-sandbox detail.
Useful? React with 👍 / 👎.
| return zap.New(zap.UseFlagOptions(&opts.ZapOptions)) | ||
| return zap.New( | ||
| zap.UseFlagOptions(&opts.ZapOptions), | ||
| zap.Encoder(zapcore.NewJSONEncoder(encoderConfig)), |
There was a problem hiding this comment.
Hard-coding a JSON encoder here overrides encoder-related options parsed from opts.BindFlags, so operators can no longer rely on documented flags like --zap-encoder (and related time-encoding choices) to control log format. This is a behavior regression from the previous implementation, where zap.UseFlagOptions determined the encoder.
Useful? React with 👍 / 👎.
1 participant
Summary
Testing
Breaking Changes
Checklist