Repository navigation
Conversation
- MarkFailure now calls AddException and propagates ActivityStatusCode.Error
to the root server span so APM shows the HTTP transaction as failed (not 200 OK)
- SearchTools returns CallToolResult { IsError = true } on exception so the
MCP client/model knows the tool actually failed
- ElasticsearchHealthCheck wires the existing CanConnect ping into /health
(returns 503 when ES is unreachable) without touching /alive (liveness)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
GitHub Actions
Failure
|
Warning Review limit reached
More reviews will be available in 31 minutes and 30 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements three interconnected improvements to the MCP remote service: adds an Elasticsearch readiness health check on the Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/Elastic.Documentation.Mcp.Remote/Telemetry/McpToolTelemetry.cs (1)
59-80:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWalk the passed tool activity’s parent chain when marking the server span failed.
MarkFailure(activity, ex)forwardsactivity, butFailServerSpancurrently traversesActivity.Current, so server-span attribution can drift if the ambient current activity differs at catch-time.Suggested fix
public static void MarkFailure(Activity? activity, Exception ex) { _ = activity?.SetTag("mcp.call.success", false); _ = activity?.SetTag("mcp.call.error_type", ex.GetType().FullName); _ = activity?.SetTag("error.message", ex.Message); _ = activity?.AddException(ex); _ = activity?.SetStatus(ActivityStatusCode.Error, ex.Message); - FailServerSpan(ex); + FailServerSpan(activity, ex); } - private static void FailServerSpan(Exception ex) + private static void FailServerSpan(Activity? activity, Exception ex) { - for (var a = Activity.Current; a is not null; a = a.Parent) + for (var a = activity?.Parent; a is not null; a = a.Parent) { if (a.Kind != ActivityKind.Server) continue; _ = a.SetStatus(ActivityStatusCode.Error, ex.Message); _ = a.AddException(ex); return; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/Elastic.Documentation.Mcp.Remote/Telemetry/McpToolTelemetry.cs` around lines 59 - 80, The failure-marking currently walks Activity.Current in FailServerSpan(Exception ex), which can misattribute server spans; change FailServerSpan to accept the activity that failed (e.g., FailServerSpan(Activity? activity, Exception ex)) and iterate up the passed activity's parent chain (for (var a = activity; a is not null; a = a.Parent)) to find the ActivityKind.Server span, then SetStatus/AddException on that span; update MarkFailure(Activity? activity, Exception ex) to call the new FailServerSpan(activity, ex) and preserve the existing null-safe behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api/Elastic.Documentation.Mcp.Remote/Health/ElasticsearchHealthCheck.cs`:
- Around line 28-30: The health check currently converts any exception into
HealthCheckResult.Unhealthy in ElasticsearchHealthCheck.CheckHealthAsync
(wrapping accessor.CanConnect which awaits Client.PingAsync), which incorrectly
swallows caller cancellation; change the catch so OperationCanceledException
(and any exception with IsCancellationRequested on the passed token) is not
converted to Unhealthy but is rethrown (or allowed to propagate) while other
exceptions still return HealthCheckResult.Unhealthy("Elasticsearch unreachable",
ex); also add a unit test that cancels the caller CancellationToken before
accessor.CanConnect completes and asserts CheckHealthAsync throws
OperationCanceledException instead of returning an Unhealthy result.
In
`@tests-integration/Mcp.Remote.IntegrationTests/SearchToolsIntegrationTests.cs`:
- Around line 28-32: Before casting and parsing the returned payload, assert
that the operation succeeded by checking result.IsError is false; in the test
methods calling searchTools.SemanticSearch (and analogous calls at the other
locations) add an assertion like Assert.False(result.IsError, $"SemanticSearch
failed: {result.Error}") immediately after awaiting the call and before
accessing result.Content or casting to TextContentBlock so failures surface
clearly instead of causing downstream JSON/DTO parsing errors.
---
Outside diff comments:
In `@src/api/Elastic.Documentation.Mcp.Remote/Telemetry/McpToolTelemetry.cs`:
- Around line 59-80: The failure-marking currently walks Activity.Current in
FailServerSpan(Exception ex), which can misattribute server spans; change
FailServerSpan to accept the activity that failed (e.g.,
FailServerSpan(Activity? activity, Exception ex)) and iterate up the passed
activity's parent chain (for (var a = activity; a is not null; a = a.Parent)) to
find the ActivityKind.Server span, then SetStatus/AddException on that span;
update MarkFailure(Activity? activity, Exception ex) to call the new
FailServerSpan(activity, ex) and preserve the existing null-safe behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2e9e0117-918b-451d-aeb0-43ba7942653a
📒 Files selected for processing (8)
src/api/Elastic.Documentation.Mcp.Remote/Elastic.Documentation.Mcp.Remote.csprojsrc/api/Elastic.Documentation.Mcp.Remote/Health/ElasticsearchHealthCheck.cssrc/api/Elastic.Documentation.Mcp.Remote/Program.cssrc/api/Elastic.Documentation.Mcp.Remote/Responses/McpToolResults.cssrc/api/Elastic.Documentation.Mcp.Remote/Telemetry/McpToolTelemetry.cssrc/api/Elastic.Documentation.Mcp.Remote/Tools/SearchTools.cstests-integration/Mcp.Remote.IntegrationTests/SearchToolsIntegrationTests.cstests/Mcp.Remote.Tests/McpToolTelemetryTests.cs
…h check, assert IsError in integration tests Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
GitHub Actions
Failure
GitHub Actions
Failure
|
This pull request is stale because it has been open 90 days with no activity. Remove the |
|
@copilot resolve the merge conflicts in this pull request Also evaluate if this fix is still necessary |
# Conflicts: # src/api/Elastic.Documentation.Mcp.Remote/Program.cs # src/api/Elastic.Documentation.Mcp.Remote/Tools/SearchTools.cs Co-authored-by: reakaleek <16325797+reakaleek@users.noreply.github.com>
Co-authored-by: reakaleek <16325797+reakaleek@users.noreply.github.com>
Merge conflicts resolved in 508ce46 (Program.cs, SearchTools.cs) plus a follow-up formatting fix — main's Re-evaluated necessity against current
All three changes remain necessary; nothing here is now redundant with main. |
Docs preview (local build)Handbook preview: https://docs-v3-preview.elastic.dev/elastic/docs-builder/pull/3466/ |
Mpdreamz
left a comment
Mpdreamz
left a comment
There was a problem hiding this comment.
I am not sure about adding ES to health.
The ECS deploy checks are SUPER finicky.
Program.cs: kept the branch's Elasticsearch readiness health check and main's reworded orchestration comment. SearchToolsIntegrationTests: main moved these tests to TUnit with TestContext.Current.Output and Execution.CancellationToken. The branch's assertions on the tool result (IsError, then the text content) are kept and use those APIs. McpToolTelemetryTests: the branch's server-span propagation tests are now [Test] instead of [Fact]. CreateListenerForSources keeps the branch's source-name set and main's named SampleAll sampler. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![infra-vault-gh-plugin-prod[bot]](https://avatars.githubusercontent.com/u/6764390?s=60&v=4){kind=small}
|
|
||
| // Readiness check: pings Elasticsearch so /health returns 503 during an ES outage. | ||
| // Excluded from /alive (liveness) to avoid pod crash-loops. | ||
| _ = builder.Services.AddHealthChecks().AddCheck<ElasticsearchHealthCheck>("elasticsearch", tags: ["ready"]); |
There was a problem hiding this comment.
Severity: P3 (Low)
The new readiness/liveness behavior has no automated coverage. Add endpoint tests with a controlled Elasticsearch backend that verify /health returns 503 on a failed or timed-out ping while /alive remains 200, plus the healthy case. Without exercising the registration and endpoint predicates together, a regression can silently restore the monitoring blind spot or make liveness depend on Elasticsearch.
Generated by Libra. React with 👍 or 👎 to give feedback on this comment.
| McpToolTelemetry.MarkFailure(activity, ex); | ||
| logger.LogError(ex, "SemanticSearch failed for query '{Query}'", query); | ||
| return JsonSerializer.Serialize(new ErrorResponse(ex.Message), McpJsonContext.Default.ErrorResponse); | ||
| return McpToolResults.Error(JsonSerializer.Serialize(new ErrorResponse(ex.Message), McpJsonContext.Default.ErrorResponse)); |
There was a problem hiding this comment.
Severity: P3 (Low)
Neither changed tool has a regression test for the new IsError = true failure result. The updated integration tests only assert successful searches and explicitly skip when Elasticsearch is unavailable, so reverting this error wrapper would leave them green. Add deterministic tests using a throwing IFullSearchService for both tools, and verify a tools/call response exposes result.isError: true while retaining HTTP 200.
Generated by Libra. React with 👍 or 👎 to give feedback on this comment.
Why
During an Elasticsearch outage the MCP search tools were silently failing — APM showed HTTP 200 everywhere,
/healthreturned healthy, and MCP clients received what looked like a successful (but empty) result. The failure was invisible to both monitoring and the calling model.What
Three gaps closed:
Transaction span propagation —
MarkFailurenow callsactivity.AddException(ex)and walks up to the ASP.NET Core server span to setActivityStatusCode.Errorthere too. The HTTP transaction now appears as a failing transaction in APM (and populates the Errors view) even though the HTTP response stays 200, which is correct per MCP/JSON-RPC spec.isErrortool results —SemanticSearchandFindRelatedDocsnow returnCallToolResult { IsError = true }on exception instead of a 200 success body containing an{"error": "..."}field. The MCP client and model now know the tool actually failed.Elasticsearch readiness check —
ElasticsearchHealthCheckwires the existingElasticsearchClientAccessor.CanConnectping into/healthso it returns 503 during an ES outage./alive(liveness) is intentionally excluded to avoid pod crash-loops.The
DefaultSearchServicein the underlying package throws with "Elasticsearch response is not valid" when ES returns an error response, so exception-based failure detection covers all ES failure modes (unreachable, error response, query rejection).Test plan
dotnet test tests/Mcp.Remote.Tests— 17 tests pass including 2 new telemetry tests verifying the server span propagationcurl /healthreturns 503,curl /alivereturns 200search_*via MCP → tool result hasisError: true, HTTP response is 200/mcpappears as a failed transaction