Add CPU profile capture #499
Conversation
WalkthroughAdds end-to-end profiling: proto messages/service (ProfileConfig, ProfileService + FlushProfiles), generated validators, a profiling package, CLI flags and default fields, runner and connector wiring to start/stop/write profiles, and a gRPC endpoint to flush profiles. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User/CLI
participant CLI as CLI
participant R as ConnectorRunner
participant CW as ConnectorWrapper
participant S as SubprocessServer
participant P as Profiler
participant PS as ProfileService (gRPC)
U->>CLI: run with profile flags
CLI->>CLI: build ProfileConfig(s) (child / optional parent)
CLI->>R: Run(..., WithProfileConfig, WithParentProfileConfig?)
R->>CW: New(..., WithProfileConfig)
CW->>S: Start subprocess (ServerConfig includes ProfileConfig)
alt child profiling enabled
CW->>P: New(cfg) / Start()
CW->>S: register ProfileService(profiler)
end
alt parent profiling enabled
R->>P: NewWithPrefix(parentCfg,"parent") / Start()
end
CW->>S: Serve()
S-->>CW: Serve terminates
alt shutdown / one-shot exit
CW->>P: Stop()
CW->>P: WriteMemProfile()
R->>PS: FlushProfiles RPC (if invoked)
PS-->>R: success/failure
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)internal/connector/profile_service.go (3)
⏰ 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). (1)
Comment |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pb/c1/connector_wrapper/v1/connector_wrapper.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
internal/connector/connector.go(6 hunks)pb/c1/connector_wrapper/v1/connector_wrapper.pb.validate.go(2 hunks)pkg/cli/commands.go(2 hunks)pkg/connectorrunner/runner.go(4 hunks)pkg/field/defaults.go(2 hunks)pkg/profiling/profiling.go(1 hunks)proto/c1/connector_wrapper/v1/connector_wrapper.proto(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
PR: ConductorOne/baton-sdk#393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter using a type switch and returns appropriate errors for nil or invalid connectors in the default case, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/connectorrunner/runner.gopkg/cli/commands.go
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
PR: ConductorOne/baton-sdk#393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter and returns appropriate errors for nil or invalid connectors, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/connectorrunner/runner.gopkg/cli/commands.go
🧬 Code graph analysis (6)
pkg/field/defaults.go (2)
pkg/field/fields.go (2)
StringField(198-214)BoolField(176-196)pkg/field/field_options.go (4)
WithDescription(47-53)WithPersistent(129-135)WithExportTarget(103-111)ExportTargetNone(97-97)
pkg/connectorrunner/runner.go (2)
pb/c1/connector_wrapper/v1/connector_wrapper.pb.go (3)
ProfileConfig(26-37)ProfileConfig(50-50)ProfileConfig(65-67)internal/connector/connector.go (2)
WithProfileConfig(130-135)Option(78-78)
pkg/profiling/profiling.go (1)
pb/c1/connector_wrapper/v1/connector_wrapper.pb.go (3)
ProfileConfig(26-37)ProfileConfig(50-50)ProfileConfig(65-67)
pkg/cli/commands.go (3)
pb/c1/connector_wrapper/v1/connector_wrapper.pb.go (3)
ProfileConfig(26-37)ProfileConfig(50-50)ProfileConfig(65-67)internal/connector/connector.go (1)
WithProfileConfig(130-135)pkg/connectorrunner/runner.go (1)
WithProfileConfig(654-659)
internal/connector/connector.go (3)
pb/c1/connector_wrapper/v1/connector_wrapper.pb.go (3)
ProfileConfig(26-37)ProfileConfig(50-50)ProfileConfig(65-67)pkg/connectorrunner/runner.go (2)
WithProfileConfig(654-659)Option(255-255)pkg/profiling/profiling.go (1)
New(27-49)
pb/c1/connector_wrapper/v1/connector_wrapper.pb.validate.go (1)
pb/c1/connector_wrapper/v1/connector_wrapper.pb.go (3)
ProfileConfig(26-37)ProfileConfig(50-50)ProfileConfig(65-67)
🪛 GitHub Actions: ci
pkg/cli/commands.go
[error] 25-25: golangci-lint: string literal not terminated (typecheck).
🪛 GitHub Check: go-lint
pkg/cli/commands.go
[failure] 25-25:
string literal not terminated (typecheck)
pkg/cli/commands.go
Outdated
| connectorwrapperV1 "github.com/conductorone/baton-sdk/pb/c1/connector_wrapper/v1" | ||
| v1 "github.com/conductorone/baton-sdk/pb/c1/connector_w |
There was a problem hiding this comment.
Fix duplicate proto import to restore build.
The import block now pulls in github.com/conductorone/baton-sdk/pb/c1/connector_wrapper/v1 twice under different aliases (connectorwrapperV1 and v1). Go forbids duplicate imports, which is why golangci-lint is failing with the “string literal not terminated” typecheck error. Drop the second import and reuse a single alias (e.g. stick with v1) for both ProfileConfig and ServerConfig.
-import (
- …
- connectorwrapperV1 "github.com/conductorone/baton-sdk/pb/c1/connector_wrapper/v1"
- v1 "github.com/conductorone/baton-sdk/pb/c1/connector_wrapper/v1"
+import (
+ …
+ v1 "github.com/conductorone/baton-sdk/pb/c1/connector_wrapper/v1"and later
- profileCfg := &connectorwrapperV1.ProfileConfig{
+ profileCfg := &v1.ProfileConfig{
OutputDir: profileDir,
EnableCpu: enableCPU,
EnableMem: enableMem,
}Apply the same alias where ProfileConfig is referenced elsewhere. Once the duplicate import is gone, the build should succeed again.
Also applies to: 303-321
🧰 Tools
🪛 GitHub Actions: ci
[error] 25-25: golangci-lint: string literal not terminated (typecheck).
🪛 GitHub Check: go-lint
[failure] 25-25:
string literal not terminated (typecheck)
🤖 Prompt for AI Agents
In pkg/cli/commands.go around lines 24-25 (and similarly lines 303-321), there
is a duplicate import of
github.com/conductorone/baton-sdk/pb/c1/connector_wrapper/v1 under two aliases
(connectorwrapperV1 and v1); remove the duplicate import and keep a single alias
(use v1), then update any places that reference connectorwrapperV1.ProfileConfig
(or connectorwrapperV1.ServerConfig) to use v1.ProfileConfig (and
v1.ServerConfig) so all references use the same import alias and the
build/typecheck error is resolved.
| f, err := os.Create(p.memFilePath) | ||
| if err != nil { | ||
| l.Error("failed to create memory profile file", zap.Error(err)) | ||
| return err | ||
| } | ||
| defer f.Close() | ||
|
|
||
| // Write heap profile without forcing GC - we want to see actual memory state | ||
| if err := pprof.WriteHeapProfile(f); err != nil { | ||
| l.Error("failed to write memory profile", zap.Error(err)) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Ensure the mem profile directory exists before creating the file
When only memory profiling is enabled (CPU disabled), outputDir is never created. os.Create on Line 109 will fail with ENOENT, so mem profiles are silently skipped. Mirror the CPU path and call os.MkdirAll(filepath.Dir(p.memFilePath), 0o755) before creating the file.
+ outputDir := filepath.Dir(p.memFilePath)
+ if err := os.MkdirAll(outputDir, 0o755); err != nil {
+ l.Error("failed to create profile output directory", zap.Error(err))
+ return err
+ }
+
f, err := os.Create(p.memFilePath)📝 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.
| f, err := os.Create(p.memFilePath) | |
| if err != nil { | |
| l.Error("failed to create memory profile file", zap.Error(err)) | |
| return err | |
| } | |
| defer f.Close() | |
| // Write heap profile without forcing GC - we want to see actual memory state | |
| if err := pprof.WriteHeapProfile(f); err != nil { | |
| l.Error("failed to write memory profile", zap.Error(err)) | |
| return err | |
| } | |
| // Ensure the mem‐profile output directory exists | |
| outputDir := filepath.Dir(p.memFilePath) | |
| if err := os.MkdirAll(outputDir, 0o755); err != nil { | |
| l.Error("failed to create profile output directory", zap.Error(err)) | |
| return err | |
| } | |
| f, err := os.Create(p.memFilePath) | |
| if err != nil { | |
| l.Error("failed to create memory profile file", zap.Error(err)) | |
| return err | |
| } | |
| defer f.Close() | |
| // Write heap profile without forcing GC - we want to see actual memory state | |
| if err := pprof.WriteHeapProfile(f); err != nil { | |
| l.Error("failed to write memory profile", zap.Error(err)) | |
| return err | |
| } |
🤖 Prompt for AI Agents
In pkg/profiling/profiling.go around lines 109 to 120, the code calls
os.Create(p.memFilePath) without ensuring the parent directory exists which
causes os.Create to fail with ENOENT when only memory profiling is enabled;
before creating the file call os.MkdirAll(filepath.Dir(p.memFilePath), 0o755)
and check its error, log and return on failure, then proceed to create the file
and write the heap profile as before.
| } | ||
|
|
||
| // New creates a new Profiler from the given configuration. | ||
| // Filenames are generated with a timestamp: cpu-YYYYMMDD-HHMMSS.prof and mem-YYYYMMDD-HHMMSS.prof |
There was a problem hiding this comment.
This uses pprof, I don't really understand what your question is here. Do you mean the file extension should be pprof instead of prof?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/profiling/profiling.go (1)
109-119: Ensure the memory profile directory exists before creating the file.When only memory profiling is enabled (CPU profiling disabled), the output directory is never created, causing
os.Createto fail withENOENT. This issue was previously identified.Apply this diff to create the directory before creating the file:
+ outputDir := filepath.Dir(p.memFilePath) + if err := os.MkdirAll(outputDir, 0o755); err != nil { + l.Error("failed to create profile output directory", zap.Error(err)) + return err + } + f, err := os.Create(p.memFilePath)
🧹 Nitpick comments (1)
pkg/profiling/profiling.go (1)
26-26: Add period to godoc comment.The godoc comment should end with a period to conform to Go documentation conventions.
Apply this diff:
-// Filenames are generated with a timestamp: cpu-YYYYMMDD-HHMMSS.prof and mem-YYYYMMDD-HHMMSS.prof +// Filenames are generated with a timestamp: cpu-YYYYMMDD-HHMMSS.prof and mem-YYYYMMDD-HHMMSS.prof.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pb/c1/connector_wrapper/v1/connector_wrapper.pb.gois excluded by!**/*.pb.gopb/c1/connector_wrapper/v1/connector_wrapper_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (8)
internal/connector/connector.go(7 hunks)internal/connector/profile_service.go(1 hunks)pb/c1/connector_wrapper/v1/connector_wrapper.pb.validate.go(3 hunks)pkg/cli/commands.go(3 hunks)pkg/connectorrunner/runner.go(6 hunks)pkg/profiling/profiling.go(1 hunks)pkg/types/types.go(1 hunks)proto/c1/connector_wrapper/v1/connector_wrapper.proto(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
PR: ConductorOne/baton-sdk#393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter and returns appropriate errors for nil or invalid connectors, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/cli/commands.gopkg/connectorrunner/runner.go
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
PR: ConductorOne/baton-sdk#393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter using a type switch and returns appropriate errors for nil or invalid connectors in the default case, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/cli/commands.gopkg/connectorrunner/runner.go
🧬 Code graph analysis (6)
internal/connector/connector.go (3)
pb/c1/connector_wrapper/v1/connector_wrapper.pb.go (6)
ProfileConfig(26-37)ProfileConfig(50-50)ProfileConfig(65-67)FlushProfilesRequest(158-162)FlushProfilesRequest(175-175)FlushProfilesRequest(190-192)pkg/profiling/profiling.go (1)
New(27-49)pb/c1/connector_wrapper/v1/connector_wrapper_grpc.pb.go (2)
RegisterProfileServiceServer(80-89)NewProfileServiceClient(38-40)
pkg/cli/commands.go (3)
pb/c1/connector_wrapper/v1/connector_wrapper.pb.go (6)
ProfileConfig(26-37)ProfileConfig(50-50)ProfileConfig(65-67)ServerConfig(90-98)ServerConfig(111-111)ServerConfig(126-128)internal/connector/connector.go (1)
WithProfileConfig(130-135)pkg/connectorrunner/runner.go (1)
WithProfileConfig(662-667)
pkg/profiling/profiling.go (1)
pb/c1/connector_wrapper/v1/connector_wrapper.pb.go (3)
ProfileConfig(26-37)ProfileConfig(50-50)ProfileConfig(65-67)
pkg/connectorrunner/runner.go (2)
pb/c1/connector_wrapper/v1/connector_wrapper.pb.go (3)
ProfileConfig(26-37)ProfileConfig(50-50)ProfileConfig(65-67)internal/connector/connector.go (2)
WithProfileConfig(130-135)Option(78-78)
internal/connector/profile_service.go (3)
pb/c1/connector_wrapper/v1/connector_wrapper_grpc.pb.go (3)
UnimplementedProfileServiceServer(66-66)UnimplementedProfileServiceServer(68-70)UnimplementedProfileServiceServer(71-71)pkg/profiling/profiling.go (1)
Profiler(18-23)pb/c1/connector_wrapper/v1/connector_wrapper.pb.go (6)
FlushProfilesRequest(158-162)FlushProfilesRequest(175-175)FlushProfilesRequest(190-192)FlushProfilesResponse(194-200)FlushProfilesResponse(213-213)FlushProfilesResponse(228-230)
pb/c1/connector_wrapper/v1/connector_wrapper.pb.validate.go (1)
pb/c1/connector_wrapper/v1/connector_wrapper.pb.go (9)
ProfileConfig(26-37)ProfileConfig(50-50)ProfileConfig(65-67)FlushProfilesRequest(158-162)FlushProfilesRequest(175-175)FlushProfilesRequest(190-192)FlushProfilesResponse(194-200)FlushProfilesResponse(213-213)FlushProfilesResponse(228-230)
🪛 GitHub Check: go-lint
pkg/profiling/profiling.go
[failure] 26-26:
Comment should end in a period (godot)
internal/connector/profile_service.go
[failure] 34-34:
error is not nil (line 33) but it returns nil (nilerr)
[failure] 26-26:
error is not nil (line 25) but it returns nil (nilerr)
⏰ 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). (1)
- GitHub Check: go-test (1.25.x, windows-latest)
🔇 Additional comments (11)
pkg/connectorrunner/runner.go (6)
30-30: LGTM!The import alias for the connector wrapper v1 package is correctly added and follows the naming conventions used elsewhere in the codebase.
137-139: LGTM!The
flushProfilesmethod correctly delegates to the client wrapper'sFlushProfilesmethod and properly propagates errors.
192-195: LGTM!The profile flushing before exit is correctly implemented with appropriate error handling. Logging the error as a warning and continuing is the right approach for best-effort cleanup during shutdown.
356-356: LGTM!The
profileConfigfield is correctly added to therunnerConfigstruct with the appropriate type.
662-667: LGTM!The
WithProfileConfigoption function follows the established pattern for other option functions and correctly stores the profile configuration.
704-706: LGTM!The profile configuration is correctly passed to the connector wrapper when provided, following the same pattern as other conditional wrapper options.
pkg/profiling/profiling.go (4)
1-15: LGTM!The package declaration and imports are appropriate. All imported packages are used in the implementation.
17-23: LGTM!The
Profilerstruct is well-designed with appropriate fields to manage CPU and memory profiling state.
51-79: LGTM!The
Startmethod correctly initializes CPU profiling with proper directory creation, error handling, and logging. The cleanup on error (closing the file ifpprof.StartCPUProfilefails) is appropriate.
81-98: LGTM!The
Stopmethod correctly stops CPU profiling, closes the file, and handles errors appropriately. NullifyingcpuFileafter closing prevents double-close issues.pb/c1/connector_wrapper/v1/connector_wrapper.pb.validate.go (1)
38-142: LGTM!The generated validation code correctly implements the protoc-gen-validate pattern for
ProfileConfig,FlushProfilesRequest, andFlushProfilesResponse. TheServerConfigvalidation has been properly extended to validate the embeddedProfileConfig. All error types and validation methods follow the standard generated structure.Also applies to: 226-253, 332-538
| // Stop CPU profiling to flush data | ||
| if err := ps.profiler.Stop(ctx); err != nil { | ||
| return &connectorwrapperV1.FlushProfilesResponse{ | ||
| Success: false, | ||
| Error: err.Error(), | ||
| }, nil | ||
| } | ||
|
|
||
| // Write memory profile | ||
| if err := ps.profiler.WriteMemProfile(ctx); err != nil { | ||
| return &connectorwrapperV1.FlushProfilesResponse{ | ||
| Success: false, | ||
| Error: err.Error(), | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Return a gRPC error instead of success=false with nil error.
FlushProfiles currently swallows the actual error and replies with success=false while returning nil for the RPC error. Apart from tripping nilerr in CI, this hides the failure from gRPC clients that only check the RPC error. Please propagate the failure as a status.Error (e.g., codes.Internal) and let callers handle it via the RPC error path.
- if err := ps.profiler.Stop(ctx); err != nil {
- return &connectorwrapperV1.FlushProfilesResponse{
- Success: false,
- Error: err.Error(),
- }, nil
- }
+ if err := ps.profiler.Stop(ctx); err != nil {
+ return nil, status.Errorf(codes.Internal, "failed to stop profiler: %v", err)
+ }
@@
- if err := ps.profiler.WriteMemProfile(ctx); err != nil {
- return &connectorwrapperV1.FlushProfilesResponse{
- Success: false,
- Error: err.Error(),
- }, nil
- }
+ if err := ps.profiler.WriteMemProfile(ctx); err != nil {
+ return nil, status.Errorf(codes.Internal, "failed to write memory profile: %v", err)
+ }Remember to add the necessary status/codes imports. After this change the success flag can stay true when no error occurs, and the lint failure should disappear. Based on static analysis hints
📝 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.
| // Stop CPU profiling to flush data | |
| if err := ps.profiler.Stop(ctx); err != nil { | |
| return &connectorwrapperV1.FlushProfilesResponse{ | |
| Success: false, | |
| Error: err.Error(), | |
| }, nil | |
| } | |
| // Write memory profile | |
| if err := ps.profiler.WriteMemProfile(ctx); err != nil { | |
| return &connectorwrapperV1.FlushProfilesResponse{ | |
| Success: false, | |
| Error: err.Error(), | |
| }, nil | |
| } | |
| // Stop CPU profiling to flush data | |
| if err := ps.profiler.Stop(ctx); err != nil { | |
| return nil, status.Errorf(codes.Internal, "failed to stop profiler: %v", err) | |
| } | |
| // Write memory profile | |
| if err := ps.profiler.WriteMemProfile(ctx); err != nil { | |
| return nil, status.Errorf(codes.Internal, "failed to write memory profile: %v", err) | |
| } |
🧰 Tools
🪛 GitHub Check: go-lint
[failure] 34-34:
error is not nil (line 33) but it returns nil (nilerr)
[failure] 26-26:
error is not nil (line 25) but it returns nil (nilerr)
🤖 Prompt for AI Agents
internal/connector/profile_service.go lines 24-38: the RPC currently returns a
FlushProfilesResponse with Success=false and a nil gRPC error when profiler.Stop
or profiler.WriteMemProfile fail, which hides the failure from gRPC clients;
change these branches to return (nil, status.Error(codes.Internal, err.Error()))
(importing "google.golang.org/grpc/status" and "google.golang.org/grpc/codes"),
and keep returning the successful response only when no error occurs so callers
receive a proper gRPC error instead of a nil error with success=false.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/connector/profile_service.go (1)
29-43: Return proper gRPC status errors instead of success=false.The past review comment correctly identifies this issue. The current implementation returns
success=falsewith a nil gRPC error, which violates gRPC conventions and triggers the nilerr lint failure. Please follow the suggested fix to returnstatus.Errorf(codes.Internal, ...)for both error cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/connector/connector.go(7 hunks)internal/connector/profile_service.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/connector/connector.go (4)
pb/c1/connector_wrapper/v1/connector_wrapper.pb.go (6)
ProfileConfig(26-39)ProfileConfig(52-52)ProfileConfig(67-69)FlushProfilesRequest(167-171)FlushProfilesRequest(184-184)FlushProfilesRequest(199-201)pkg/connectorrunner/runner.go (2)
WithProfileConfig(680-685)Option(280-280)pkg/profiling/profiling.go (1)
New(28-36)pb/c1/connector_wrapper/v1/connector_wrapper_grpc.pb.go (2)
RegisterProfileServiceServer(80-89)NewProfileServiceClient(38-40)
internal/connector/profile_service.go (3)
pb/c1/connector_wrapper/v1/connector_wrapper_grpc.pb.go (3)
UnimplementedProfileServiceServer(66-66)UnimplementedProfileServiceServer(68-70)UnimplementedProfileServiceServer(71-71)pkg/profiling/profiling.go (1)
Profiler(18-23)pb/c1/connector_wrapper/v1/connector_wrapper.pb.go (6)
FlushProfilesRequest(167-171)FlushProfilesRequest(184-184)FlushProfilesRequest(199-201)FlushProfilesResponse(203-209)FlushProfilesResponse(222-222)FlushProfilesResponse(237-239)
🪛 GitHub Check: go-lint
internal/connector/profile_service.go
[failure] 39-39:
error is not nil (line 38) but it returns nil (nilerr)
[failure] 31-31:
error is not nil (line 30) but it returns nil (nilerr)
⏰ 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). (1)
- GitHub Check: go-test (1.25.x, windows-latest)
🔇 Additional comments (4)
internal/connector/connector.go (4)
130-135: LGTM!The option follows the established pattern and correctly sets the profile configuration.
162-171: LGTM!The profiling initialization is correctly placed before the gRPC server startup to capture the full lifecycle, with appropriate error handling and logging.
205-209: LGTM!The ProfileService registration follows the same conditional pattern as other feature-gated services in this file.
238-238: LGTM!The ProfileConfig is correctly propagated to the subprocess server configuration.
| // FlushProfiles calls the ProfileService RPC to flush any active profiling data. | ||
| func (cw *wrapper) FlushProfiles(ctx context.Context) error { | ||
| cw.mtx.RLock() | ||
| conn := cw.conn | ||
| cw.mtx.RUnlock() | ||
|
|
||
| if conn == nil { | ||
| return fmt.Errorf("no active connection") | ||
| } | ||
|
|
||
| client := connectorwrapperV1.NewProfileServiceClient(conn) | ||
| resp, err := client.FlushProfiles(ctx, &connectorwrapperV1.FlushProfilesRequest{}) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if !resp.Success { | ||
| return fmt.Errorf("profile flush failed: %s", resp.Error) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Update for compatibility with the profile_service.go gRPC error fix.
Once profile_service.go is fixed to return proper gRPC errors (as suggested in the previous review), this method will break. When the service returns an error via status.Errorf, resp will be nil and lines 385-387 will cause a panic or become unreachable.
Apply this diff to handle errors correctly:
func (cw *wrapper) FlushProfiles(ctx context.Context) error {
cw.mtx.RLock()
conn := cw.conn
cw.mtx.RUnlock()
if conn == nil {
return fmt.Errorf("no active connection")
}
client := connectorwrapperV1.NewProfileServiceClient(conn)
- resp, err := client.FlushProfiles(ctx, &connectorwrapperV1.FlushProfilesRequest{})
- if err != nil {
- return err
- }
-
- if !resp.Success {
- return fmt.Errorf("profile flush failed: %s", resp.Error)
- }
-
- return nil
+ _, err := client.FlushProfiles(ctx, &connectorwrapperV1.FlushProfilesRequest{})
+ return err
}This simplifies the method to rely on the gRPC error convention where errors are returned via the error return value, not via response fields.
🤖 Prompt for AI Agents
In internal/connector/connector.go around lines 369-390, FlushProfiles currently
assumes a non-nil resp and checks resp.Success/resp.Error which will break once
the service returns gRPC errors via status.Errorf (resp will be nil). Change the
function to follow gRPC conventions: call client.FlushProfiles, if err != nil
return err, otherwise return nil (remove reliance on resp.Success/resp.Error);
keep the early conn nil check and mutex usage as-is and ensure no
nil-dereference of resp occurs.
Summary by CodeRabbit
New Features
Bug Fixes / Validation