EPMRPP-110724 || MCP Server. GA Analytics. Add RP instanceId parameter#77
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds lazy, retried fetching of ReportPortal instance ID into Analytics: Changes
Sequence DiagramsequenceDiagram
participant Client
participant Analytics
participant RP_API
participant GA4_API
Client->>Analytics: sendBatchMetrics()
Analytics->>Analytics: ensureInstanceID()
alt instanceID not fetched
Analytics->>RP_API: GET /api/info (5s timeout)
alt success
RP_API-->>Analytics: JSON with extensions.result["server.details.instance"]
Analytics->>Analytics: parse & store instanceID (atomic set)
else fail/timeout
RP_API-->>Analytics: error/timeout
Analytics->>Analytics: log, leave unset (retry later)
end
end
Analytics->>GA4_API: POST batch (events include instanceID when present)
GA4_API-->>Analytics: response
Analytics-->>Client: return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (4)
🧰 Additional context used📓 Path-based instructions (4)**/*.go📄 CodeRabbit inference engine (README.md)
Files:
⚙️ CodeRabbit configuration file
Files:
**/internal/reportportal/**/*.go📄 CodeRabbit inference engine (README.md)
Files:
internal/reportportal/**⚙️ CodeRabbit configuration file
Files:
**/*_test.go⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (2)📚 Learning: 2026-01-15T11:38:28.032ZApplied to files:
📚 Learning: 2026-01-15T11:38:28.032ZApplied to files:
🧬 Code graph analysis (1)internal/reportportal/server.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). (1)
🔇 Additional comments (10)
✏️ Tip: You can disable this entire section by setting 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.
Pull request overview
This PR adds ReportPortal instance ID parameter tracking to Google Analytics events. The instance ID is fetched from the ReportPortal /api/info endpoint and included as a parameter in GA4 events to help identify which ReportPortal instance generated the analytics data.
Changes:
- Added lazy-loading mechanism to fetch ReportPortal instance ID from
/api/infoendpoint on first metrics processing - Updated
NewAnalyticsfunction signature to acceptrpHostURLparameter for instance ID fetching - Added instance ID parameter to Google Analytics events when available
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/reportportal/analytics.go | Core implementation of instance ID fetching with retry logic and lazy loading, added instance ID parameter to GA events |
| internal/reportportal/server.go | Updated NewAnalytics call to include host URL parameter |
| internal/reportportal/http_server.go | Updated NewAnalytics call to include host URL parameter with comment |
| internal/reportportal/analytics_test.go | Added comprehensive test coverage for instance ID fetching scenarios and updated all NewAnalytics calls |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/reportportal/http_server.go (1)
101-108: Potential nil pointer dereference ifconfig.HostURLis nil.While
config.HostURLis also used on line 140 withgorp.NewClient(), the analytics initialization happens earlier. Ifconfig.HostURLis nil when analytics is enabled, calling.String()will panic.Consider adding a defensive nil check or validating
config.HostURLat the start ofNewHTTPServer:🛡️ Suggested defensive check
func NewHTTPServer(config HTTPServerConfig) (*HTTPServer, error) { + // Validate required configuration + if config.HostURL == nil { + return nil, fmt.Errorf("HostURL is required") + } + // Set defaults if config.MaxConcurrentRequests <= 0 {
🤖 Fix all issues with AI agents
In `@internal/reportportal/analytics_test.go`:
- Around line 1088-1089: The test HTTP handler ignores the error returned by
w.Write, which the linter will flag; update the two occurrences of
w.Write([]byte(response)) (and the similar call at line 1204) to explicitly
discard the error by assigning it to the blank identifier (e.g., _ =
w.Write(...)) so the return value is handled and the linter warning is silenced.
- Around line 1138-1177: The test creates a mock GA server but never points the
analytics code at it; update the test to override the GA endpoint before sending
events (e.g., set the package-level ga4EndpointURL to gaServer.URL and defer
restoring the original value), then call analytics.TrackMCPEvent(ctx,
"test_tool") and analytics.processMetrics() and assert that capturedEvents (the
slice populated by the mock server) contains at least one event and that each
event includes analytics.instanceID (or mockInstanceID); reference
TrackMCPEvent, processMetrics, ga4EndpointURL, capturedEvents and mockInstanceID
when locating the spots to change.
In `@internal/reportportal/analytics.go`:
- Around line 166-168: The apiURL construction can produce a double slash when
hostURL ends with '/', so before building apiURL in the function that sets
apiURL (the fmt.Sprintf("%s/api/info", hostURL) line) trim any trailing slash
from hostURL (e.g., use strings.TrimSuffix(hostURL, "/")) and then format the
URL; also add "strings" to the imports if missing so the fix compiles.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
internal/reportportal/analytics.gointernal/reportportal/analytics_test.gointernal/reportportal/http_server.gointernal/reportportal/server.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (README.md)
**/*.go: Build and compile the ReportPortal MCP Server Go application usinggo build -o reportportal-mcp-server ./cmd/reportportal-mcp-server
Set environment variables RP_HOST, RP_PROJECT, and RP_API_TOKEN for stdio mode operation, or set MCP_MODE=http with MCP_SERVER_PORT and MCP_SERVER_HOST for HTTP mode
Require Go 1.24.1 or later for building the ReportPortal MCP Server
Add new tools by creating methods in resource files and registering them in theNewServerfunction usings.AddTool()
Ensure all MCP requests follow JSON-RPC 2.0 specification with required fields: jsonrpc, method, id, and params
Use/mcpor/api/mcpendpoints for POST requests (MCP protocol); use/,/health,/info,/api/status, or/metricsfor GET requests only
Files:
internal/reportportal/http_server.gointernal/reportportal/analytics_test.gointernal/reportportal/server.gointernal/reportportal/analytics.go
⚙️ CodeRabbit configuration file
Focus on Go best practices, error handling, concurrency safety, performance, and security. Check for proper resource cleanup and context handling.
Files:
internal/reportportal/http_server.gointernal/reportportal/analytics_test.gointernal/reportportal/server.gointernal/reportportal/analytics.go
internal/reportportal/**
⚙️ CodeRabbit configuration file
Pay special attention to API integration patterns, error handling, data validation, and MCP protocol compliance.
Files:
internal/reportportal/http_server.gointernal/reportportal/analytics_test.gointernal/reportportal/server.gointernal/reportportal/analytics.go
**/*_test.go
⚙️ CodeRabbit configuration file
Ensure comprehensive test coverage, proper test isolation, meaningful assertions, and good test naming conventions.
Files:
internal/reportportal/analytics_test.go
🧬 Code graph analysis (2)
internal/reportportal/analytics_test.go (2)
internal/reportportal/analytics.go (3)
NewAnalytics(241-294)GAEvent(88-91)GAPayload(94-99)internal/reportportal/server.go (1)
NewServer(19-80)
internal/reportportal/server.go (1)
internal/reportportal/analytics.go (1)
NewAnalytics(241-294)
🔇 Additional comments (11)
internal/reportportal/server.go (1)
44-44: LGTM!The change correctly passes the host URL string to
NewAnalytics, aligning with the updated signature. The existing code already assumeshostUrlis non-nil (as seen in line 36 where it's used directly withgorp.NewClient), so no additional nil check is needed here.internal/reportportal/analytics.go (6)
38-40: LGTM!A 5-second timeout is reasonable for fetching instance metadata from the ReportPortal
/api/infoendpoint.
106-111: LGTM!Good use of the atomic.Bool + sync.Mutex pattern for efficient double-check locking. The atomic flag enables a lock-free fast path for the common case (already fetched), while the mutex ensures thread-safe initialization.
123-156: LGTM!The double-check locking pattern is correctly implemented:
- Fast-path atomic check avoids locking overhead when already fetched
- Lock acquisition, then re-check to prevent redundant fetches
- Only marks as fetched on success, enabling retry on failure
The retry-until-success design is appropriate for a non-critical metadata fetch.
202-231: LGTM!The JSON parsing with type assertions is properly guarded. Each step validates the expected structure before proceeding, with informative warning logs for debugging when the expected fields are missing or malformed.
277-289: LGTM!Clean initialization of the new instance ID fields. The explicit empty string for
instanceIDenhances readability. The HTTP client's 10-second timeout is appropriately longer than the context-basedinstanceIDFetchTimeout(5s), allowing the context to control the actual request deadline.
624-654: LGTM!The instance ID integration is thread-safe:
ensureInstanceID()is called to trigger lazy fetch- The atomic flag check before reading
instanceIDis correct—Go's atomic operations provide the necessary memory ordering guarantees- Conditionally adding
instanceIDonly when available is a clean approachinternal/reportportal/analytics_test.go (4)
6-6: LGTM!The
encoding/jsonimport is correctly added to support the new instance ID fetching tests that decode GA payloads.
152-152: LGTM!All
NewAnalyticscalls are correctly updated with the empty string fourth parameter, maintaining backward-compatible behavior in existing tests.
1179-1241: LGTM!Excellent retry logic test coverage:
- Validates failure on first two attempts
- Confirms success on third attempt
- Importantly verifies that subsequent calls use the cached value without triggering additional HTTP requests
The mutex-protected counter correctly handles potential concurrent access in the HTTP handler.
1243-1258: LGTM!Good edge case coverage: verifies that when
rpHostURLis empty,ensureInstanceID()returns early without attempting a fetch and doesn't mark the ID as fetched—maintaining correct retry behavior if a host URL is later configured.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
AleksandrParamonoff
force-pushed
the
add-new-metric-parameter
branch
3 times, most recently
from
a7f46b6 to
e917db1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
AleksandrParamonoff
force-pushed
the
add-new-metric-parameter
branch
from
e917db1 to
7faf1ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/reportportal/http_server.go`:
- Around line 75-78: The inline comment attached to the function signature for
NewHTTPServer is causing golines formatting errors; move the comment to its own
line above the function signature or delete it entirely and leave the validation
block (the HostURL nil check) as-is so the function signature is on a single
line and golines passes.
♻️ Duplicate comments (2)
internal/reportportal/analytics_test.go (1)
1142-1183: Test doesn't verify instanceID is actually sent in GA events.The mock GA server (
gaServer) is created but never used—events are still sent to the real GA4 endpoint. The test only verifies thatinstanceIDis stored in the analytics struct, not that it appears in the event payload.Consider either:
- Removing the unused
gaServerandcapturedEventsto clarify the test's actual scope- Or injecting the mock GA endpoint URL into the analytics config to verify end-to-end behavior
♻️ Simplified test focusing on what's actually verified
t.Run("instance ID is added to GA events", func(t *testing.T) { - // Create a mock GA server to capture events - var capturedEvents []GAEvent - var mu sync.Mutex - gaServer := httptest.NewServer( - http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.Method == "POST" { - var payload GAPayload - if err := json.NewDecoder(r.Body).Decode(&payload); err == nil { - mu.Lock() - capturedEvents = append(capturedEvents, payload.Events...) - mu.Unlock() - } - w.WriteHeader(http.StatusOK) - } - }), - ) - defer gaServer.Close() - // Create analytics with mock RP server analytics, err := NewAnalytics("test-user", "test-secret", "", mockServer.URL) require.NoError(t, err) require.NotNil(t, analytics) defer analytics.Stop() - // Override GA endpoint for testing (we need to modify the config) // Track an event ctx := context.Background() analytics.TrackMCPEvent(ctx, "test_tool") - // Wait a bit for batching - time.Sleep(100 * time.Millisecond) - // Process metrics manually analytics.processMetrics() - // Verify events were created with instanceID - // Note: In real scenario, events would be sent to GA server - // For this test, we verify the instanceID is stored in analytics + // Verify the instanceID was fetched and stored (would be included in events) assert.True(t, analytics.instanceIDFetched.Load(), "Should be fetched after processing") assert.Equal(t, mockInstanceID, analytics.instanceID) })internal/reportportal/analytics.go (1)
649-652: Inconsistent parameter naming: use snake_case for consistency.The parameter name
instanceIDuses camelCase while other parameters in the same map (custom_user_id,event_name,tool) use snake_case. For consistency and to match GA4 conventions, this should beinstance_id.🔧 Suggested fix
// Add instanceID if available if currentInstanceID != "" { - params["instanceID"] = currentInstanceID + params["instance_id"] = currentInstanceID }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
internal/reportportal/analytics.gointernal/reportportal/analytics_test.gointernal/reportportal/http_server.gointernal/reportportal/server.go
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (README.md)
MCP protocol requests must follow the JSON-RPC 2.0 specification with fields: jsonrpc, method, id, and params
Files:
internal/reportportal/analytics_test.gointernal/reportportal/server.gointernal/reportportal/analytics.gointernal/reportportal/http_server.go
⚙️ CodeRabbit configuration file
Focus on Go best practices, error handling, concurrency safety, performance, and security. Check for proper resource cleanup and context handling.
Files:
internal/reportportal/analytics_test.gointernal/reportportal/server.gointernal/reportportal/analytics.gointernal/reportportal/http_server.go
**/internal/reportportal/**/*.go
📄 CodeRabbit inference engine (README.md)
When adding new tools, create a new method in the appropriate resource file and add it to the server in the NewServer function
Files:
internal/reportportal/analytics_test.gointernal/reportportal/server.gointernal/reportportal/analytics.gointernal/reportportal/http_server.go
**/*_test.go
⚙️ CodeRabbit configuration file
Ensure comprehensive test coverage, proper test isolation, meaningful assertions, and good test naming conventions.
Files:
internal/reportportal/analytics_test.go
internal/reportportal/**
⚙️ CodeRabbit configuration file
Pay special attention to API integration patterns, error handling, data validation, and MCP protocol compliance.
Files:
internal/reportportal/analytics_test.gointernal/reportportal/server.gointernal/reportportal/analytics.gointernal/reportportal/http_server.go
🧠 Learnings (2)
📚 Learning: 2026-01-15T11:38:28.032Z
Learnt from: CR
Repo: reportportal/reportportal-mcp-server PR: 0
File: README.md:0-0
Timestamp: 2026-01-15T11:38:28.032Z
Learning: Applies to **/server/**/*.go : In HTTP mode, authentication tokens must be passed per-request via Authorization: Bearer header, and RP_API_TOKEN environment variable is not used
Applied to files:
internal/reportportal/analytics_test.go
📚 Learning: 2026-01-15T11:38:28.032Z
Learnt from: CR
Repo: reportportal/reportportal-mcp-server PR: 0
File: README.md:0-0
Timestamp: 2026-01-15T11:38:28.032Z
Learning: Applies to **/internal/reportportal/**/*.go : When adding new tools, create a new method in the appropriate resource file and add it to the server in the NewServer function
Applied to files:
internal/reportportal/server.gointernal/reportportal/http_server.go
🧬 Code graph analysis (2)
internal/reportportal/analytics_test.go (2)
internal/reportportal/analytics.go (3)
NewAnalytics(237-295)GAEvent(89-92)GAPayload(95-100)internal/reportportal/server.go (1)
NewServer(19-80)
internal/reportportal/server.go (1)
internal/reportportal/analytics.go (1)
NewAnalytics(237-295)
🪛 GitHub Actions: Build
internal/reportportal/http_server.go
[error] 75-75: File is not properly formatted (golines).
🪛 GitHub Check: build
internal/reportportal/http_server.go
[failure] 75-75:
File is not properly formatted (golines)
⏰ 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: Call feature Docker build / Build and export to AWS ECR
🔇 Additional comments (15)
internal/reportportal/analytics.go (6)
39-41: LGTM! Appropriate timeout constant for instance ID fetching.The 5-second timeout is reasonable for a network call to fetch the instance ID, balancing responsiveness with allowing for network latency.
107-112: Well-structured lazy initialization fields.The combination of
atomic.Boolfor fast-path checking andsync.Mutexfor protecting the actual write is a solid pattern for lazy initialization with thread safety.
124-157: Correct double-checked locking implementation.The fast-path atomic check followed by mutex acquisition and re-check is the canonical pattern for thread-safe lazy initialization in Go. Good use of early return when
rpHostURLis empty.
159-227: Robust instance ID fetching with proper resource cleanup.Good implementation with:
- Context-based timeout
- Proper response body closure with error logging
- Defensive JSON parsing with type assertions at each level
237-242: Clean signature extension and initialization.The new
rpHostURLparameter is properly stored for lazy fetching, and the sharedhttpClientis correctly initialized with a sensible timeout.Also applies to: 278-290
631-637: Good fix for memory visibility on instanceID read.Acquiring the mutex after the atomic check ensures proper memory visibility when reading
instanceID. This addresses the potential race condition where another goroutine could be mid-write.internal/reportportal/http_server.go (1)
107-112: Correct propagation of host URL to analytics.The host URL is properly passed to
NewAnalyticsfor instance ID fetching. Good use of.String()to convert the*url.URLto a string parameter.internal/reportportal/analytics_test.go (7)
6-6: LGTM! Import added for JSON decoding in tests.The
encoding/jsonimport is needed for the new instance ID fetching tests that decode JSON payloads.
152-152: All NewAnalytics call sites correctly updated.The fourth parameter (empty string for
hostURL) has been consistently added to all test invocations, maintaining backward compatibility for tests that don't need instance ID functionality.Also applies to: 354-354, 507-507, 516-516, 572-572, 622-622, 658-658, 699-699, 729-729, 766-766, 815-815, 821-821, 896-896, 902-902, 965-965, 1027-1027
1074-1094: Well-designed mock server for instance ID testing.The mock ReportPortal server correctly returns the expected JSON structure with the instance ID at
extensions.result["server.details.instance"]. Good use ofhttptest.NewServerfor isolation.
1096-1111: Good coverage of lazy loading behavior.This test correctly verifies that:
- Instance ID is not fetched on initialization
- Instance ID is fetched when
ensureInstanceID()is called- The fetched value matches the mock server response
1113-1140: Comprehensive test of lazy fetch trigger during metrics processing.Good verification that instance ID fetch is triggered by
processMetrics()rather thanTrackMCPEvent(), confirming the lazy loading design.
1185-1254: Excellent retry behavior test coverage.This test thoroughly validates:
- Failed attempts don't mark instanceID as fetched
- Successful fetch after failures works correctly
- Subsequent calls use cached value (no additional HTTP requests)
The use of a counter to verify caching behavior is particularly good.
1256-1279: Good edge case coverage for empty host URL.Verifies that when no host URL is provided,
ensureInstanceID()gracefully handles the case without attempting network calls or marking the ID as fetched.internal/reportportal/server.go (1)
44-44: Remove this comment - the nil pointer concern is invalid.The caller in
cmd/reportportal-mcp-server/main.govalidateshostUrlwithurl.Parse()and error checking (lines 264-266) before callingNewServer, ensuring the URL is never nil. Additionally, ifhostUrlwere nil, the dereference would fail at line 36 (gorp.NewClient(hostUrl, token)) before reaching line 44, since Go'surl.Parse()cannot return a nil pointer with a nil error. The function safely uses the validated parameter.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
AleksandrParamonoff
force-pushed
the
add-new-metric-parameter
branch
from
7faf1ba to
c17eca1
Compare
AleksandrParamonoff
force-pushed
the
add-new-metric-parameter
branch
from
c17eca1 to
76323a2
Compare
No description provided.