feat: add opentelemtry integration in filter flag #5247
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds telemetry instrumentation to worktree discovery, git operations, and filter evaluation workflows. It introduces a new telemetry module with operation name constants and wrapper functions, integrates telemetry into the discovery and worktree lifecycle flows, adds repository metadata query methods to GitRunner, and includes integration tests for the telemetry functions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 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 |
| repoCommit := gitRunner.GetHeadCommit(ctx) | ||
|
|
||
| // Wrap entire worktree creation process with telemetry | ||
| _ = filter.TraceGitWorktreesCreate(ctx, workingDir, len(gitRefs), repoRemote, repoBranch, repoCommit, func(ctx context.Context) error { |
There was a problem hiding this comment.
We should warn on this error instead of ignoring it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
internal/filter/telemetry_test.go (1)
168-222: Consider adding test for error propagation.The uniqueness validation is good. Consider adding a test case that verifies error propagation when the callback returns an error (without telemeter), ensuring errors are not swallowed.
func TestTraceFunction_ErrorPropagation(t *testing.T) { t.Parallel() expectedErr := errors.New("test error") err := filter.TraceGitWorktreeCreate(context.Background(), "main", "/tmp", "", "", "", func(ctx context.Context) error { return expectedErr }) require.ErrorIs(t, err, expectedErr, "error should propagate from callback") }internal/discovery/worktreediscovery.go (1)
175-184: Consider aligning metric names with constants.The metric names
"git_worktree_discovery_worktree_pairs"and"git_worktree_discovery_components"are hardcoded strings. Consider defining these as constants in the telemetry module alongside the operation names for consistency and to prevent typos.internal/worktrees/worktrees.go (2)
365-471: Discarded error fromTraceGitWorktreesCreateis intentional but could be clearer.The return value is discarded (
_ =) because errors are captured viaouterErr. This works correctly but might confuse future readers. A brief comment explaining this design would improve clarity.- // Wrap entire worktree creation process with telemetry - _ = filter.TraceGitWorktreesCreate(ctx, workingDir, len(gitRefs), repoRemote, repoBranch, repoCommit, func(ctx context.Context) error { + // Wrap entire worktree creation process with telemetry. + // The trace function's error is intentionally ignored since errors are captured in outerErr + // and returned after the trace completes. + _ = filter.TraceGitWorktreesCreate(ctx, workingDir, len(gitRefs), repoRemote, repoBranch, repoCommit, func(ctx context.Context) error {
476-486: Consider aligning metric names with constants.Similar to
worktreediscovery.go, the metric names here ("git_diff_files_added", etc.) are hardcoded strings. Consider defining these as constants in the telemetry module for consistency withTelemetryOp*andAttr*constants.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/discovery/worktreediscovery.go(3 hunks)internal/filter/telemetry.go(1 hunks)internal/filter/telemetry_test.go(1 hunks)internal/git/git.go(1 hunks)internal/worktrees/worktrees.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
Files:
internal/git/git.gointernal/filter/telemetry_test.gointernal/discovery/worktreediscovery.gointernal/filter/telemetry.gointernal/worktrees/worktrees.go
🧠 Learnings (3)
📚 Learning: 2025-02-10T13:36:19.542Z
Learnt from: levkohimins
Repo: gruntwork-io/terragrunt PR: 3723
File: cli/commands/stack/action.go:160-160
Timestamp: 2025-02-10T13:36:19.542Z
Learning: The project uses a custom error package `github.com/gruntwork-io/terragrunt/internal/errors` which provides similar functionality to `fmt.Errorf` but includes stack traces. Prefer using this package's error functions (e.g., `errors.Errorf`, `errors.New`) over the standard library's error handling.
Applied to files:
internal/discovery/worktreediscovery.gointernal/worktrees/worktrees.go
📚 Learning: 2025-08-19T16:05:54.723Z
Learnt from: Resonance1584
Repo: gruntwork-io/terragrunt PR: 4683
File: go.mod:86-90
Timestamp: 2025-08-19T16:05:54.723Z
Learning: When analyzing Go module dependencies for removal, always check for both direct imports and API usage across all Go files in the repository, not just a quick search. The github.com/mattn/go-zglob library is used for filesystem walking and glob expansion in multiple Terragrunt files including util/file.go, format commands, and AWS provider patch functionality.
Applied to files:
internal/discovery/worktreediscovery.gointernal/worktrees/worktrees.go
📚 Learning: 2025-04-17T13:02:28.098Z
Learnt from: yhakbar
Repo: gruntwork-io/terragrunt PR: 4169
File: cli/commands/hcl/validate/cli.go:29-60
Timestamp: 2025-04-17T13:02:28.098Z
Learning: Avoid shadowing imported packages with local variables in Go code, such as using a variable named `flags` when the `github.com/gruntwork-io/terragrunt/cli/flags` package is imported. Use more specific variable names like `flagSet` instead.
Applied to files:
internal/discovery/worktreediscovery.gointernal/worktrees/worktrees.go
🧬 Code graph analysis (3)
internal/filter/telemetry_test.go (1)
internal/filter/telemetry.go (40)
TraceGitWorktreeCreate(57-80)TraceGitWorktreeRemove(83-93)TraceGitWorktreesCreate(96-119)TraceGitWorktreesCleanup(122-136)TraceGitDiff(139-154)TraceGitWorktreeDiscovery(157-166)TraceGitWorktreeStackWalk(169-179)TraceFilterEvaluate(182-192)TraceFilterParse(195-204)TraceGitFilterExpand(207-220)TraceGitFilterEvaluate(223-234)TraceGraphFilterTraverse(237-247)TelemetryOpGitWorktreeCreate(13-13)TelemetryOpGitWorktreeRemove(14-14)TelemetryOpGitWorktreesCreate(15-15)TelemetryOpGitWorktreesCleanup(16-16)TelemetryOpGitDiff(17-17)TelemetryOpGitWorktreeDiscovery(18-18)TelemetryOpGitWorktreeStackWalk(19-19)TelemetryOpGitWorktreeFilterApply(20-20)TelemetryOpFilterEvaluate(23-23)TelemetryOpFilterParse(24-24)TelemetryOpGitFilterExpand(25-25)TelemetryOpGitFilterEvaluate(26-26)TelemetryOpGraphFilterTraverse(27-27)AttrGitRef(32-32)AttrGitFromRef(33-33)AttrGitToRef(34-34)AttrGitWorktreeDir(35-35)AttrGitWorkingDir(36-36)AttrGitRefCount(37-37)AttrGitDiffAdded(38-38)AttrGitDiffRemoved(39-39)AttrGitDiffChanged(40-40)AttrFilterQuery(48-48)AttrFilterType(49-49)AttrFilterCount(50-50)AttrComponentCount(51-51)AttrResultCount(52-52)AttrWorktreePairCount(53-53)
internal/filter/telemetry.go (1)
telemetry/context.go (1)
TelemeterFromContext(23-31)
internal/worktrees/worktrees.go (1)
internal/filter/telemetry.go (5)
TraceGitWorktreesCleanup(122-136)TraceGitWorktreeRemove(83-93)TraceGitWorktreesCreate(96-119)TraceGitDiff(139-154)TraceGitWorktreeCreate(57-80)
⏰ 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). (4)
- GitHub Check: build / Build (windows/amd64)
- GitHub Check: base_tests / Test (ubuntu)
- GitHub Check: base_tests / Test (macos)
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (9)
internal/git/git.go (1)
455-497: LGTM! Clean implementation following existing patterns.The three new methods (
GetRemoteURL,GetCurrentBranch,GetHeadCommit) appropriately return empty strings on error, which is suitable for telemetry attributes where missing data is acceptable. This is consistent with the existingHasUncommittedChangespattern.internal/filter/telemetry_test.go (1)
12-166: LGTM! Good coverage of no-telemeter fallback paths.The tests properly verify that all
Trace*functions execute their callbacks even when no telemeter is present in the context. This is critical for ensuring the instrumentation doesn't break functionality when telemetry is disabled.internal/discovery/worktreediscovery.go (2)
73-173: LGTM! Well-structured telemetry integration.The telemetry wrapper properly encapsulates the entire discovery flow. The use of
TraceGitWorktreeDiscoveryandTraceGitFilterExpandprovides good observability into the discovery process. TherecordWorktreeDiscoveryMetricshelper correctly checks for nil telemeter.
298-316: LGTM! Clean extraction pattern for telemetry wrapping.The separation of
walkChangedStack(telemetry wrapper) andwalkChangedStackInternal(actual logic) follows a clean pattern that keeps the telemetry concern separate from the business logic.internal/worktrees/worktrees.go (2)
117-138: LGTM! Proper telemetry integration in cleanup with graceful error handling.The cleanup correctly wraps individual worktree removals with
TraceGitWorktreeRemoveand handles already-cleaned-up worktrees gracefully by checking error messages. The use oftgerrors.Errorffollows the project's custom error package convention.
532-535: LGTM! Telemetry attributes include repo context.The worktree creation correctly includes repository metadata (
repoRemote,repoBranch,repoCommit) in telemetry, providing valuable context for debugging and observability.internal/filter/telemetry.go (3)
10-54: LGTM! Well-organized telemetry constants.The operation names and attribute keys follow a consistent naming convention (
git.*,filter.*,component.*,worktree.*,result.*). The separation into operation names and attribute keys provides clear structure.
82-247: LGTM! Consistent wrapper implementations.All wrapper functions follow the same pattern: check for telemeter, build attributes map (with conditional optional attributes), and delegate to
telemeter.Collect. This consistency makes the code predictable and maintainable.
56-61: No issues found - nil check is effective and correctly implemented.TelemeterFromContext returns nil when no telemeter exists in the context (not a non-nil empty struct as initially claimed). The nil checks in the wrapper functions are necessary and function as intended, allowing callers to skip telemetry collection when it's not configured.
Likely an incorrect or invalid review comment.
| TelemetryOpGitWorktreeStackWalk = "git_worktree_stack_walk" | ||
| TelemetryOpGitWorktreeFilterApply = "git_worktree_filter_apply" |
There was a problem hiding this comment.
Unused constant: TelemetryOpGitWorktreeFilterApply.
The constant TelemetryOpGitWorktreeFilterApply is defined but there is no corresponding TraceGitWorktreeFilterApply wrapper function. Either add the wrapper function or remove the unused constant.
🤖 Prompt for AI Agents
In internal/filter/telemetry.go around lines 19-20, the constant
TelemetryOpGitWorktreeFilterApply is defined but unused; add a corresponding
TraceGitWorktreeFilterApply wrapper function (matching the style/signature of
other Trace... wrappers in this file) that starts and returns the telemetry span
using TelemetryOpGitWorktreeFilterApply, accepts the same parameters (context,
name/attrs as used by the other wrappers), and ensures the span is finished or
returned consistently, or alternatively remove the
TelemetryOpGitWorktreeFilterApply constant if you prefer not to add the wrapper.
| AttrGitDiffChanged = "git.diff.changed_count" | ||
|
|
||
| // Repository identification attributes | ||
| AttrGitRepoRoot = "git.repo.root" |
There was a problem hiding this comment.
Unused constant: AttrGitRepoRoot.
The attribute key AttrGitRepoRoot is defined but not used in any of the telemetry wrapper functions. Consider removing it if it's not planned for immediate use, or document its intended use case.
🤖 Prompt for AI Agents
In internal/filter/telemetry.go around line 43, the constant AttrGitRepoRoot
("git.repo.root") is defined but unused; either remove the constant to eliminate
dead code or document its intended telemetry use and add it to the relevant
wrapper functions where repository root should be recorded (or create a TODO
comment referencing where it will be used). If you choose to keep it, add a
short comment describing its purpose and add its use in the telemetry attribute
map where git-related attributes are set so the linter no longer flags it as
unused.
Description
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
Refactor
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.