chore: Replace sort package with slices and cmp packages - #986
chore: Replace sort package with slices and cmp packages#986dongjiang1989 wants to merge 1 commit into
Conversation
Replace all usage of Go's sort package with the modern slices and cmp packages (Go 1.21+) for more concise and readable sorting code. - Remove sort.Interface methods (Len/Less/Swap) from LabelNames, LabelValues, LabelPairs, Fingerprints, Alerts, Samples, Vector, and Matrix types. - Add Compare methods on *Sample, *SampleStream, *Alert, and *LabelPair for use with slices.SortFunc. - Replace sort.Sort calls with slices.SortFunc using the new Compare methods, and sort.Strings with slices.Sort. - Add depguard rule in .golangci.yml to forbid the sort package and prevent future regressions. - Fix pre-existing gofumpt formatting issues in config and route packages. Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
|
NOTE: This is a breaking change for downstream - we should do it ideally only when we have to |
There was a problem hiding this comment.
🟢 Approval recommended
The sorting refactor is consistent and test updates align with the new comparison logic, with only minor doc-comment nits remaining.
Pull request overview
This PR modernizes sorting across the codebase by replacing sort usage with Go 1.21+ slices/cmp, and centralizing comparison logic into Compare methods on key model types.
Changes:
- Replaced
sort.Sort/sort.Stringsusage withslices.SortFunc/slices.Sortacross model and expfmt code. - Introduced
Comparemethods on*Sample,*SampleStream,*Alert, and*LabelPairto reuse consistent ordering logic. - Added a
depguardrule to forbid importingsort, preventing reintroduction.
File summaries
| File | Description |
|---|---|
| route/route_test.go | Formatting-only adjustments to method chaining (gofumpt style). |
| model/value.go | Adds Compare methods and migrates matrix sorting to slices.SortFunc. |
| model/value_test.go | Updates vector sorting test to use slices.SortFunc with (*Sample).Compare. |
| model/metric.go | Replaces sort.Strings with slices.Sort for label string ordering. |
| model/labelset.go | Replaces label-name sorting with slices.SortFunc + cmp.Compare. |
| model/labels.go | Removes sort.Interface impls and adds (*LabelPair).Compare. |
| model/labels_test.go | Migrates label sorting tests to slices.SortFunc. |
| model/fingerprinting.go | Removes sort.Interface impl from Fingerprints (now sorted via slices elsewhere). |
| model/fingerprinting_test.go | Migrates fingerprint sorting test to slices.SortFunc + cmp.Compare. |
| model/alert.go | Replaces Alerts.Less with a strict, hierarchical (*Alert).Compare. |
| model/alert_test.go | Updates alert sorting test to use slices.SortFunc and new expected order. |
| expfmt/decode_test.go | Updates sample sorting in decoder tests to use (*model.Sample).Compare. |
| config/tls_config_test.go | Formatting-only cast simplification for TLSVersion. |
| config/http_config.go | Formatting-only cast simplification and gofumpt-style trailing commas in varargs calls. |
| .golangci.yml | Adds depguard deny rule for sort imports. |
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Compare implements the cmp.Comparator interface for Alert pointers. | ||
| // It compares by StartsAt, then EndsAt, then Fingerprint. |
| // Compare implements the cmp.Comparator interface for LabelPair pointers. | ||
| // It compares by Name, then by Value. |
| // Compare implements the cmp.Comparator interface for Sample pointers. | ||
| // It compares first the metrics, then the timestamp. |
| Histograms []SampleHistogramPair `json:"histograms"` | ||
| } | ||
|
|
||
| // Compare implements the cmp.Comparator interface for SampleStream pointers. |
bboreham
left a comment
There was a problem hiding this comment.
Agree with Bartek - it's quite a breaking change to make all these types no longer implement sort.Interface. Suggest to leave them in, maybe with a comment that they are for backwards-compatibility only. You could have Less call the new Compare.
Can you explain a bit more about the change to Alert ordering? This will be another breaking change; we'd need a strong justification.
Fix pre-existing gofumpt formatting issues in config and route packages (uncovered by make lint).
Do this in a separate PR. It's extra work to review changes for multiple different reasons at the same time.
Also, it might be neater to use cmp.Or in the Compare methods.
| "TLS12": (TLSVersion)(tls.VersionTLS12), | ||
| "TLS11": (TLSVersion)(tls.VersionTLS11), | ||
| "TLS10": (TLSVersion)(tls.VersionTLS10), | ||
| "TLS13": TLSVersion(tls.VersionTLS13), |
There was a problem hiding this comment.
This change is not covered by the PR title.
| conntrack.DialWithName(name), | ||
| ) |
|
Thanks for the review @bwplotka @bboreham. Understood. I'll restore Len/Less/Swap methods as a compatibility layer, delegating to the new Compare(). I will make these changes in the next push:
|
Replace all usage of Go's
sortpackage with the modernslicesandcmppackages (Go 1.21+) for more concise and readable sorting code.Changes
sort.Interfacemethods (Len/Less/Swap) fromLabelNames,LabelValues,LabelPairs,Fingerprints,Alerts,Samples,Vector, andMatrixtypes.Comparemethods on*Sample,*SampleStream,*Alert, and*LabelPairfor use withslices.SortFunc, eliminating duplicated comparator code at call sites.sort.Sortcalls withslices.SortFunc(using the newComparemethods orcmp.Comparefor simple ordered types), andsort.Stringswithslices.Sort.depguardrule in.golangci.ymlto forbid thesortpackage and prevent future regressions.gofumptformatting issues inconfigandroutepackages (uncovered bymake lint).Note
The original
Alerts.Lesswas not a proper strict weak ordering (it usedStartsBefore || EndsBefore || FPLess). The new(*Alert).Compareimplements a correct hierarchical comparison (StartsAt → EndsAt → Fingerprint).TestSortAlertsexpected values were updated accordingly.Verification
go build ./...✅go vet ./...✅make lint✅go test ./model/... ./expfmt/...✅