feat(ext): add observability extension hooks - #666
Open
SantiagoDePolonia wants to merge 5 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThe pull request adds upstream observer extensions, stream-first-chunk hooks, provider and operation metadata propagation, passthrough semantic enrichment, outer middleware wiring, and centralized metrics endpoint resolution. ChangesObservability and request metadata
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@config/metrics.go`:
- Around line 22-31: Update ResolveMetricsEndpoint to normalize non-empty
configured endpoints as absolute paths before the reserved /v1 and /p prefix
checks. Ensure relative values such as metrics, v1/models, and ../v1/models are
converted and cleaned into absolute paths, while preserving the existing
conflict fallback to /metrics.
In `@internal/llmclient/client.go`:
- Around line 655-663: Update DoPassthrough so observeFirstChunk is called only
for successful HTTP responses, never for terminal retryable or non-retryable
error responses, while preserving response closure and retry handling. Add a
regression test covering a streaming passthrough request whose error response
body does not trigger OnStreamFirstChunk.
In `@internal/providers/credentials.go`:
- Line 328: Add test coverage for ManagedProviderCredential.buildProvider, using
a named managed provider and factory-installed hooks; assert that both request
and response hook metadata contain the expected Provider and ProviderType
identity after ProviderFactory.Create().
In `@internal/providers/openrouter/passthrough_semantics_test.go`:
- Around line 17-23: Extend the semantic assertions in
internal/providers/openrouter/passthrough_semantics_test.go:17-23 to require
info.AuditPath == "/v1/chat/completions", and in
internal/providers/zai/passthrough_semantics_test.go:17-23 to require
info.AuditPath == "/v1/embeddings"; keep the existing provider, GenAI operation,
and semantic operation checks.
In `@internal/server/http.go`:
- Around line 225-235: In the metrics setup around MetricsEnabled, reject
MetricsEndpoint values equal to or beneath the /debug/pprof route family when
pprof is enabled before appending metricsPath to authSkipPaths. Preserve the
existing route-conflict handling for other endpoints, and add an integration
test covering metrics and pprof enabled together.
- Around line 261-268: Register middleware.Recover() immediately after
redactSensitiveRequestURI() and before the cfg.OuterMiddleware loop in the HTTP
middleware setup, then remove its later registration so outer middleware panics
are recovered. Add a test covering a panic from outer middleware and assert that
the server returns the recovered response.
In `@internal/server/translated_inference_service.go`:
- Around line 453-459: Add the ProviderName field to the fast-path request
construction alongside Model and Stream, populating it with
providerNameFromWorkflow(workflow). Preserve the existing provider-routing
fields and behavior.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0e82fcf1-e9af-4d02-911f-a118cb4efedc
📒 Files selected for processing (63)
config/metrics.goconfig/metrics_test.goext/ext.goext/registry.goext/registry_test.goext/upstream.gointernal/app/app.gointernal/app/app_test.gointernal/core/passthrough.gointernal/core/semantic.gointernal/core/semantic_test.gointernal/llmclient/client.gointernal/llmclient/client_test.gointernal/llmclient/hooks.gointernal/llmclient/hooks_test.gointernal/llmclient/operations.gointernal/providers/anthropic/anthropic.gointernal/providers/anthropic/chat.gointernal/providers/anthropic/chat_stream.gointernal/providers/anthropic/passthrough_semantics.gointernal/providers/anthropic/responses.gointernal/providers/bedrock/bedrock.gointernal/providers/bedrock/bedrock_test.gointernal/providers/bedrock/chat.gointernal/providers/bedrock/chat_stream.gointernal/providers/bedrock/observation.gointernal/providers/cohere/chat.gointernal/providers/cohere/cohere.gointernal/providers/cohere/cohere_test.gointernal/providers/cohere/embeddings.gointernal/providers/cohere/passthrough_semantics.gointernal/providers/cohere/passthrough_semantics_test.gointernal/providers/config.gointernal/providers/config_test.gointernal/providers/credentials.gointernal/providers/deepseek/passthrough_semantics.gointernal/providers/factory.gointernal/providers/factory_test.gointernal/providers/gemini/gemini.gointernal/providers/kilo/passthrough_semantics.gointernal/providers/ollama/ollama.gointernal/providers/openai/compatible_provider.gointernal/providers/openai/passthrough_semantics.gointernal/providers/openai/passthrough_semantics_test.gointernal/providers/openrouter/openrouter.gointernal/providers/openrouter/passthrough_semantics.gointernal/providers/openrouter/passthrough_semantics_test.gointernal/providers/passthrough.gointernal/providers/sglang/passthrough_semantics.gointernal/providers/sglang/sglang.gointernal/providers/vertex/vertex.gointernal/providers/vertex/vertex_test.gointernal/providers/vllm/passthrough_semantics.gointernal/providers/vllm/vllm.gointernal/providers/zai/passthrough_semantics.gointernal/providers/zai/passthrough_semantics_test.gointernal/providers/zai/zai.gointernal/server/handlers_test.gointernal/server/http.gointernal/server/http_test.gointernal/server/passthrough_semantic_enrichment_test.gointernal/server/passthrough_service.gointernal/server/translated_inference_service.go
| } | ||
|
|
||
| cfg := buildProviderConfig(rawCfg, s.resilience) | ||
| cfg.Name = name |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
test_file="$(fd '^credentials_test\.go$' internal/providers | head -n1)"
test -n "$test_file"
rg -n -C 5 'buildProvider|install\(|SetHooks|OnRequestStart|OnRequestEnd|ProviderType|Provider:' \
internal/providers/credentials.go "$test_file"Repository: ENTERPILOT/GoModel
Length of output: 6270
🏁 Script executed:
#!/bin/bash
set -euo pipefail
test_file="$(fd '^credentials_test\.go$' internal/providers | head -n1)"
echo "test_file=$test_file"
echo "--- credentials.go relevant sections ---"
sed -n '220,350p' internal/providers/credentials.go
echo "--- config_test relevant sections ---"
if [ -f internal/providers/config_test.go ]; then
sed -n '1,260p' internal/providers/config_test.go
fi
echo "--- managed provider hook registrations ---"
rg -n -C 4 'ProviderFactory|Create\(|OnRequestStart|OnRequestEnd|RequestHooks|ResponseHooks|ProviderType|Provider:' internal/providersRepository: ENTERPILOT/GoModel
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- providers test files ---"
fd '_test\.go$' internal/providers | sed 's#^\./##' | sort
echo "--- focused hook tests in provider tests ---"
rg -n -C 4 'OnRequestStart|OnRequestEnd|SetHooks|ProviderOptions|RequestHooks|ResponseHooks|Hooks' internal/providers -g '*_test.go' | head -n 220
echo "--- ProviderFactory definition and Create implementation ---"
rg -n -C 3 'type ProviderFactory|func \(.*ProviderFactory\).*Create|ProviderOptions|RequestHooks|ResponseHooks|NewProviderFactory|func NewProviderFactory' internal/providers -g '*.go' | head -n 260Repository: ENTERPILOT/GoModel
Length of output: 37404
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- provider factory files ---"
fd 'factory|provider' internal/providers -t f | grep -E '(^|/)(factory|provider)(\.go|/)' | sort
echo "--- factory.go outline/relevant ---"
file="$(fd '^factory\.go$' internal/providers | head -n1)"
echo "file=$file"
wc -l "$file"
if [ -n "$file" ]; then
sed -n '1,260p' "$file"
fi
echo "--- credentials_test.go outline/relevant ---"
creds_test="$(fd '^credentials_test\.go$' internal/providers | head -n1)"
wc -l "$creds_test"
rg -n -C 4 'buildProvider|Register|^func Test|ProviderConfig|Name|ConfiguredProviders|Reload|Add\(|Registration|ProviderFactory|Enabled|models' "$creds_test"Repository: ENTERPILOT/GoModel
Length of output: 19430
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- core types containing ProviderInfo/RequestInfo/ResponseInfo/Hooks ---"
rg -n -C 4 'type ProviderInfo|type RequestInfo|type ResponseInfo|type Hooks|ProviderName|ProviderType|ProviderInfo' internal -g '*.go' | head -n 180
echo "--- llmclient hook request execution ---"
for f in $(fd '.*\.go$' internal/llmclient); do
if rg -q 'OnRequestStart|OnRequestEnd|OnStreamFirstChunk|ProviderInfo|RequestInfo|ResponseInfo' "$f"; then
echo "### $f"
rg -n -C 3 'OnRequestStart|OnRequestEnd|OnStreamFirstChunk|ProviderInfo|RequestInfo|ResponseInfo|ProviderName|ProviderType' "$f"
fi
doneRepository: ENTERPILOT/GoModel
Length of output: 24899
Add coverage for managed-provider hook identity.
ManagedProviderCredential.buildProvider() assigns cfg.Name before ProviderFactory.Create() decorates hooks, so named managed providers can carry provider identity in hook metadata. Add a test that installs a named managed provider with factory hooks and asserts Provider and ProviderType in request/response hook metadata.
🤖 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 `@internal/providers/credentials.go` at line 328, Add test coverage for
ManagedProviderCredential.buildProvider, using a named managed provider and
factory-installed hooks; assert that both request and response hook metadata
contain the expected Provider and ProviderType identity after
ProviderFactory.Create().
Source: Coding guidelines
Confidence Score: 5/5No blocking failure remains. The previously reported passthrough streaming behavior is covered by the current propagation of uncertain stream intent and by successful upstream SSE response detection. An end-to-end execution confirmed that the observer receives the first response chunk under the reported request conditions.
 T-Rex LogsWhat T-Rex did
Reviews (2): Last reviewed commit: "fix(ext): harden telemetry extension hoo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/app/app_test.go (1)
787-830: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPass the resolved Prometheus route to middleware factories.
applyExtensionscreatesext.HTTPServerConfig{MetricsEndpoint: serverCfg.MetricsEndpoint}beforeconfig.ResolveMetricsEndpointremoves reserved conflicts. Use the resolved path for fallback and pprof-conflict cases:""/"/v1"and pprof-conflicting values such as"/debug/pprof"should propagate as/metricstoOuterMiddlewareFor. Cover these as table cases inTestApplyExtensionsSnapshotsRegistryIntoServerConfig.🤖 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 `@internal/app/app_test.go` around lines 787 - 830, Update applyExtensions to resolve the metrics endpoint before constructing ext.HTTPServerConfig, so fallback values such as empty or "/v1" and pprof-conflicting paths like "/debug/pprof" are passed to OuterMiddlewareFor as "/metrics". Extend TestApplyExtensionsSnapshotsRegistryIntoServerConfig with table-driven cases covering each resolved outcome, while preserving existing endpoint propagation.
🤖 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 `@internal/app/app.go`:
- Around line 120-122: Update the app server setup around applyExtensions and
server.New to resolve the effective metrics endpoint once, including
normalization and pprof-conflict fallback, then reuse that value for both
extensions.OuterMiddlewareFor and route registration. Add coverage for the
default path, a custom path without a leading slash, and a pprof-conflicting
path.
In `@internal/providers/credentials_test.go`:
- Around line 85-119: Extend the hook propagation test around factory.Add and
providerHooks to configure and invoke OnStreamFirstChunk, then assert its
captured RequestInfo has Provider "managed-eu" and ProviderType "test",
alongside the existing OnRequestStart and OnRequestEnd identity checks.
In `@internal/server/handlers_test.go`:
- Around line 2245-2250: Expand the passthrough request assertions in the
affected test to a table-driven check covering Operation, Model, Stream, and
ProviderName. Validate each field against the expected translated fast-path
request produced by tryFastPathStreamingChatPassthrough, while retaining the
existing nil-request guard.
In `@internal/server/request_selector_peek.go`:
- Line 31: Update the bounded request-body peek logic around the stream decoder
and core.MarkPassthroughStreamUncertain: track whether the stream field was
encountered or parsed, and mark StreamUncertain only when that field was not
parsed, not merely when complete remains false. Add a table-driven test covering
stream appearing before model and provider, preserving the existing uncertainty
behavior when stream is absent.
---
Outside diff comments:
In `@internal/app/app_test.go`:
- Around line 787-830: Update applyExtensions to resolve the metrics endpoint
before constructing ext.HTTPServerConfig, so fallback values such as empty or
"/v1" and pprof-conflicting paths like "/debug/pprof" are passed to
OuterMiddlewareFor as "/metrics". Extend
TestApplyExtensionsSnapshotsRegistryIntoServerConfig with table-driven cases
covering each resolved outcome, while preserving existing endpoint propagation.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fe9bb1da-1e5a-42ca-912c-ce3960cc6e0d
📒 Files selected for processing (24)
config/metrics.goconfig/metrics_test.goext/registry.goext/upstream.gointernal/app/app.gointernal/app/app_test.gointernal/core/passthrough.gointernal/core/semantic.gointernal/llmclient/client.gointernal/llmclient/client_test.gointernal/providers/anthropic/anthropic.gointernal/providers/cohere/cohere.gointernal/providers/credentials_test.gointernal/providers/openai/compatible_provider.gointernal/providers/openrouter/passthrough_semantics_test.gointernal/providers/sglang/sglang.gointernal/providers/vllm/vllm.gointernal/providers/zai/passthrough_semantics_test.gointernal/server/handlers_test.gointernal/server/http.gointernal/server/http_test.gointernal/server/passthrough_service.gointernal/server/request_selector_peek.gointernal/server/translated_inference_service.go
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/providers/credentials_test.go (1)
81-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse table-driven cases for hook identity propagation.
Represent request start, request end, and first chunk as table cases. Invoke each callback and assert the captured provider name and type. This keeps new hook cases consistent with the test convention.
🤖 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 `@internal/providers/credentials_test.go` around lines 81 - 128, Refactor TestCredentialsService_BuildProviderPreservesManagedHookIdentity to use table-driven cases for OnRequestStart, OnRequestEnd, and OnStreamFirstChunk. For each case, invoke the corresponding providerHooks callback and assert the captured RequestInfo or ResponseInfo has provider “managed-eu” and type “test,” preserving the existing identity checks while applying the same assertion pattern consistently.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@internal/providers/credentials_test.go`:
- Around line 81-128: Refactor
TestCredentialsService_BuildProviderPreservesManagedHookIdentity to use
table-driven cases for OnRequestStart, OnRequestEnd, and OnStreamFirstChunk. For
each case, invoke the corresponding providerHooks callback and assert the
captured RequestInfo or ResponseInfo has provider “managed-eu” and type “test,”
preserving the existing identity checks while applying the same assertion
pattern consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e1723e86-9130-4345-bad9-812a497b46b3
📒 Files selected for processing (11)
config/metrics.goconfig/metrics_test.gointernal/app/app.gointernal/app/app_test.gointernal/providers/config.gointernal/providers/config_test.gointernal/providers/credentials_test.gointernal/server/handlers_test.gointernal/server/http.gointernal/server/request_selector_peek.gointernal/server/request_selector_peek_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/server/request_snapshot_test.go`:
- Around line 176-229: Extend
TestStoreRequestBodySnapshot_PreservesPassthroughEnrichment into a table-driven
test with both parseable and malformed/opaque request-body cases. For the
parsing-failure case, assert that the existing passthrough route info preserves
Model, Stream, and StreamUncertain unchanged; retain the current
successful-refresh assertions for the parseable case.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 22d6a2cf-4cb8-4e57-8a50-d26b9be7628c
📒 Files selected for processing (8)
internal/auditlog/enrich.gointernal/core/semantic.gointernal/server/handlers_test.gointernal/server/passthrough_execution_helpers.gointernal/server/passthrough_execution_helpers_test.gointernal/server/passthrough_service.gointernal/server/request_snapshot.gointernal/server/request_snapshot_test.go
Comment on lines
+176
to
+229
| func TestStoreRequestBodySnapshot_PreservesPassthroughEnrichment(t *testing.T) { | ||
| e := echo.New() | ||
| padding := strings.Repeat("x", int(requestSnapshotInlineBodyLimit)+1) | ||
| reqBody := `{"model":"stub-model","padding":"` + padding + `","stream":true}` | ||
| req := httptest.NewRequest(http.MethodPost, "/p/vllm/chat/completions", strings.NewReader(reqBody)) | ||
| req.Header.Set("Content-Type", "application/json") | ||
| rec := httptest.NewRecorder() | ||
| c := e.NewContext(req, rec) | ||
|
|
||
| snapshot := core.NewRequestSnapshot( | ||
| http.MethodPost, | ||
| "/p/vllm/chat/completions", | ||
| nil, | ||
| nil, | ||
| nil, | ||
| "application/json", | ||
| nil, | ||
| false, | ||
| "", | ||
| nil, | ||
| ) | ||
| prompt := core.DeriveWhiteBoxPrompt(snapshot) | ||
| require.NotNil(t, prompt) | ||
| core.CachePassthroughRouteInfo(prompt, &core.PassthroughRouteInfo{ | ||
| Provider: "vllm", | ||
| ProviderName: "vllm-eu", | ||
| RawEndpoint: "chat/completions", | ||
| NormalizedEndpoint: "chat/completions", | ||
| SemanticOperation: "vllm.chat_completions", | ||
| GenAIOperation: "chat", | ||
| StreamUncertain: true, | ||
| AuditPath: "/v1/chat/completions", | ||
| Model: "stub-model", | ||
| }) | ||
| ctx := core.WithRequestSnapshot(req.Context(), snapshot) | ||
| ctx = core.WithWhiteBoxPrompt(ctx, prompt) | ||
| c.SetRequest(req.WithContext(ctx)) | ||
|
|
||
| storeRequestBodySnapshot(c, []byte(reqBody)) | ||
|
|
||
| refreshed := core.GetWhiteBoxPrompt(c.Request().Context()) | ||
| require.NotNil(t, refreshed) | ||
| info := refreshed.CachedPassthroughRouteInfo() | ||
| require.NotNil(t, info) | ||
| assert.Equal(t, "vllm", info.Provider) | ||
| assert.Equal(t, "vllm-eu", info.ProviderName) | ||
| assert.Equal(t, "chat/completions", info.NormalizedEndpoint) | ||
| assert.Equal(t, "vllm.chat_completions", info.SemanticOperation) | ||
| assert.Equal(t, "chat", info.GenAIOperation) | ||
| assert.Equal(t, "/v1/chat/completions", info.AuditPath) | ||
| assert.Equal(t, "stub-model", info.Model) | ||
| assert.True(t, info.Stream) | ||
| assert.False(t, info.StreamUncertain) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Cover the unparseable-body refresh path with a table-driven test.
This test covers only JSONBodyParsed == true. Add a malformed or opaque body case. Verify that the prior Model, Stream, and StreamUncertain values remain unchanged when parsing fails.
As per coding guidelines, “Add or update table-driven tests for behavior changes.”
🤖 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 `@internal/server/request_snapshot_test.go` around lines 176 - 229, Extend
TestStoreRequestBodySnapshot_PreservesPassthroughEnrichment into a table-driven
test with both parseable and malformed/opaque request-body cases. For the
parsing-failure case, assert that the existing passthrough route info preserves
Model, Stream, and StreamUncertain unchanged; retain the current
successful-refresh assertions for the parseable case.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds the Core extension seams needed for Pro-owned observability without adding an OpenTelemetry dependency to Core.
AI Generated (optional)
Implemented and reviewed with Codex.
Testing
Summary by CodeRabbit