Add linter rule for missing defer perf.Track() calls#1698
Conversation
- Created new perf-track rule in lintroller custom linter - Checks that all public functions have defer perf.Track() at start - Skips test files, mock files, mock types, and logger package - Provides helpful error message with suggested function name format - Added test coverage for the new rule - Updated .golangci.yml with perf-track rule (disabled by default) - Rule disabled in standalone mode to avoid blocking commits - Can be enabled by setting perf-track: true in .golangci.yml settings - Follows coding guidelines for mandatory performance tracking Note: Using --no-verify due to pre-existing os.Args linter violations that are unrelated to this change and should be fixed separately. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Enabled perf-track rule by default in both golangci-lint and standalone mode - Added comprehensive package exclusions: logger, profiler, perf, store, ui, tui - Added receiver type exclusions for TUI models, error types, and simple helpers - Exclusions prevent infinite recursion and avoid tracking overhead in low-level code - Rule now catches missing perf.Track() calls in business logic while excluding infrastructure code - All tests pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a new PerfTrackRule linter that reports missing leading Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Plugin as Lintroller Plugin
participant Rule as PerfTrackRule
participant AST as AST Walker
participant Pass as analysis.Pass
Plugin->>Rule: Check(pass, file)
Rule->>AST: Walk file AST
AST->>AST: Find exported funcs/methods with bodies
alt exported & not excluded
AST->>Pass: Inspect first statement
alt first statement is defer perf.Track(...)
Pass-->>Rule: compliant (no diagnostic)
else missing defer perf.Track
Rule->>Rule: buildPerfTrackName(pkgPath, receiver, func)
Rule-->>Pass: Report diagnostic + suggested defer perf.Track("<name>")
end
else skipped (unexported / excluded pkg / test/generated)
AST-->>Rule: skip
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (3)**/testdata/**📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Files:
**/*.go📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Files:
**/!(*_test).go📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Files:
🧠 Learnings (3)📚 Learning: 2025-10-22T06:25:54.400ZApplied to files:
📚 Learning: 2025-10-11T19:06:16.131ZApplied to files:
📚 Learning: 2025-10-13T18:13:54.020ZApplied to files:
🧬 Code graph analysis (1)tools/lintroller/testdata/src/perftrack/example.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (4)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tools/lintroller/rule_perf_track.go (2)
95-103: Consider extracting receiver exclusion check for clarity.The condition at line 99 inline-checks multiple patterns. Extracting this to a helper function would improve readability.
Consider this refactor:
+// isExcludedReceiver checks if a receiver type should be excluded from perf tracking. +func isExcludedReceiver(receiverType string, excludedList []string) bool { + for _, excluded := range excludedList { + if receiverType == excluded || strings.HasPrefix(receiverType, "mock") || strings.HasPrefix(receiverType, "Mock") { + return true + } + } + return false +} + if !hasPerfTrack { // Get receiver type if it's a method. receiverType := "" if funcDecl.Recv != nil && len(funcDecl.Recv.List) > 0 { receiverType = formatReceiverType(funcDecl.Recv.List[0].Type) - // Check if receiver type is in exclusion list. - for _, excluded := range excludedReceivers { - if receiverType == excluded || strings.HasPrefix(receiverType, "mock") || strings.HasPrefix(receiverType, "Mock") { - return true - } + if isExcludedReceiver(receiverType, excludedReceivers) { + return true } }
120-134: Add comment explaining the double-call pattern.The function correctly detects
perf.Track()(), but a brief comment explaining why we check for nested CallExpr would help future maintainers understand the pattern.// isPerfTrackCall checks if a call expression is perf.Track(). func isPerfTrackCall(call *ast.CallExpr) bool { - // Check for perf.Track()(). + // Check for perf.Track()() - the inner Track() returns a function that's immediately invoked. + // In defer context: defer perf.Track(atmosConfig, "name")() outerCall, ok := call.Fun.(*ast.CallExpr) if !ok { return false }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.golangci.yml(1 hunks)tools/lintroller/lintroller_test.go(1 hunks)tools/lintroller/plugin.go(6 hunks)tools/lintroller/rule_perf_track.go(1 hunks)tools/lintroller/testdata/src/perftrack/example.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: Define interfaces for major functionality (interface-driven design) to enable testability and decoupling
Generate mocks with go.uber.org/mock/mockgen using //go:generate directives; do not write manual mocks
Prefer the functional Options pattern to avoid functions with many parameters
Use context.Context only for cancellation, deadlines/timeouts, and sparing request-scoped values; never for config or dependencies
When used, context.Context must be the first parameter of functions
All comments must end with periods (godot linter)
Organize imports in three groups (stdlib, third-party, atmos) separated by blank lines and sorted alphabetically; maintain aliases cfg, log, u, errUtils
Add defer perf.Track(atmosConfig, "pkg.FuncName")() with a blank line in all public functions (use nil if no atmosConfig)
Use Viper for configuration loading with precedence: CLI flags → ENV vars → config files → defaults
Use static errors from errors/errors.go; wrap with fmt.Errorf and errors.Join; check with errors.Is; never compare error strings or create dynamic errors
Add //go:generate mockgen directives near interfaces and generate mocks; never hand-write mocks
Keep files focused and under 600 lines; one cmd/impl per file; co-locate tests; never disable revive file-length limit
Bind environment variables with viper.BindEnv and use ATMOS_ prefix
UI (prompts/status) must be written to stderr; data to stdout; logging only for system events, never for UI
Ensure cross-platform compatibility (Linux/macOS/Windows); use SDKs over binaries and filepath.Join() for paths
Files:
tools/lintroller/plugin.gotools/lintroller/lintroller_test.gotools/lintroller/testdata/src/perftrack/example.gotools/lintroller/rule_perf_track.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
tools/lintroller/plugin.gotools/lintroller/testdata/src/perftrack/example.gotools/lintroller/rule_perf_track.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Use table-driven tests for coverage and clarity
Test behavior, not implementation; avoid tautological/stub tests; use DI for testability
Tests must call actual production code paths; never duplicate logic in tests
Use t.Skipf("reason") with clear context when skipping tests; CLI tests auto-build temp binaries
Files:
tools/lintroller/lintroller_test.go
.golangci.yml
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Configure golangci-lint with gofmt, goimports, govet, staticcheck, errcheck, ineffassign, misspell, unused, revive, gocritic enabled
Files:
.golangci.yml
**/testdata/**
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Use test fixtures for complex inputs under testdata
Files:
tools/lintroller/testdata/src/perftrack/example.go
🧠 Learnings (4)
📓 Common learnings
Learnt from: osterman
PR: cloudposse/atmos#1599
File: pkg/ui/markdown/renderer.go:247-259
Timestamp: 2025-10-11T19:06:16.131Z
Learning: Performance tracking with `defer perf.Track()` should be reserved for functions that perform actual computational work, I/O operations, or have measurable performance impact. Simple wrapper methods that immediately delegate to other functions do not require performance tracking, as it adds unnecessary overhead without providing meaningful insights.
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T01:54:30.645Z
Learning: Applies to **/*.go : Add defer perf.Track(atmosConfig, "pkg.FuncName")() with a blank line in all public functions (use nil if no atmosConfig)
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to .golangci.yml : Configure golangci-lint with gofmt, goimports, govet, staticcheck, errcheck, ineffassign, misspell, unused, revive, gocritic enabled
Applied to files:
.golangci.yml
📚 Learning: 2025-10-22T01:54:30.645Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T01:54:30.645Z
Learning: Applies to **/*.go : Add defer perf.Track(atmosConfig, "pkg.FuncName")() with a blank line in all public functions (use nil if no atmosConfig)
Applied to files:
.golangci.ymltools/lintroller/testdata/src/perftrack/example.gotools/lintroller/rule_perf_track.go
📚 Learning: 2025-10-11T19:06:16.131Z
Learnt from: osterman
PR: cloudposse/atmos#1599
File: pkg/ui/markdown/renderer.go:247-259
Timestamp: 2025-10-11T19:06:16.131Z
Learning: Performance tracking with `defer perf.Track()` should be reserved for functions that perform actual computational work, I/O operations, or have measurable performance impact. Simple wrapper methods that immediately delegate to other functions do not require performance tracking, as it adds unnecessary overhead without providing meaningful insights.
Applied to files:
tools/lintroller/testdata/src/perftrack/example.gotools/lintroller/rule_perf_track.go
🧬 Code graph analysis (3)
tools/lintroller/plugin.go (1)
tools/lintroller/rule_perf_track.go (1)
PerfTrackRule(12-12)
tools/lintroller/lintroller_test.go (1)
tools/lintroller/plugin.go (1)
Analyzer(13-17)
tools/lintroller/testdata/src/perftrack/example.go (1)
pkg/perf/perf.go (1)
Track(121-138)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build (windows)
- GitHub Check: Run pre-commit hooks
- GitHub Check: autofix
- GitHub Check: Analyze (go)
- GitHub Check: Review Dependency Licenses
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
🔇 Additional comments (11)
.golangci.yml (1)
129-136: Configuration looks good.The perf-track rule is properly enabled and documented. The inline comment at line 136 clearly explains the rule's purpose and references the exclusion lists in the implementation file.
tools/lintroller/rule_perf_track.go (2)
11-38: Exclusion lists are well-documented and comprehensive.The excluded packages and receivers cover the key infrastructure code where perf tracking would cause recursion or unnecessary overhead. The inline comments clearly explain the rationale for each exclusion.
137-164: Helper functions are well-implemented.Both
formatReceiverTypeandbuildPerfTrackNamecorrectly handle their respective tasks. The logic properly handles pointer receivers, value receivers, and constructs appropriate perf.Track names for both functions and methods.tools/lintroller/lintroller_test.go (1)
13-15: Test structure looks good.The test properly uses the analysistest framework to exercise the perf-track rule with dedicated testdata. This follows the established testing pattern used by other rules in the file.
tools/lintroller/testdata/src/perftrack/example.go (1)
1-41: Test data is well-structured and comprehensive.The testdata correctly demonstrates both compliant and non-compliant cases, including:
- Functions with proper perf.Track calls
- Functions missing perf.Track (with want annotations)
- Methods on types
- Private functions (correctly not checked)
Note that the good examples use
perf.Track(nil, ...)which aligns with the coding guideline to use nil when atmosConfig is unavailable.tools/lintroller/plugin.go (6)
15-15: Documentation properly updated.The analyzer doc string correctly reflects the new perf.Track checks alongside existing rules.
28-28: PerfTrackRule properly integrated into standalone mode.The rule is correctly included in the standalone rules list, ensuring it runs when using the analyzer directly.
49-49: Settings field properly defined.The PerfTrack field is correctly added with appropriate JSON and YAML tags, matching the pattern of other rule settings.
65-71: Default initialization logic correctly updated.The condition at line 65 properly checks for PerfTrack, and line 71 enables it by default when no explicit settings are provided. This ensures the rule is active by default while allowing users to disable it if needed.
82-82: Documentation update is consistent.The doc string matches the update in the standalone analyzer documentation.
112-114: Conditional execution properly implemented.The rule is correctly instantiated and added to the rules list only when enabled in settings, following the same pattern as other rules in this function.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1698 +/- ##
==========================================
+ Coverage 66.74% 66.76% +0.02%
==========================================
Files 364 364
Lines 42499 42499
==========================================
+ Hits 28365 28374 +9
+ Misses 12045 12036 -9
Partials 2089 2089
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
- Analyze function signature to detect atmosConfig parameter - Suggest `perf.Track(atmosConfig, ...)` when parameter exists - Suggest `perf.Track(nil, ...)` when parameter doesn't exist - Added helper function `hasAtmosConfigParam()` to detect parameter - Updated tests to verify both scenarios work correctly - Provides more accurate, actionable suggestions to developers 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
These changes were released in v1.196.0-rc.0. |
what
perf-tracklinter rule to catch missingdefer perf.Track()callswhy
references
CLAUDE.mdfor mandatorydefer perf.Track()usage🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation