Copilot commented on code in PR #13317: URL: https://github.com/apache/apisix/pull/13317#discussion_r3159259783
########## t/plugin/ai-proxy-upstream-vars.t: ########## @@ -0,0 +1,118 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +=encoding utf-8 + +Validates that upstream nginx variables ($upstream_status, $upstream_addr, +$upstream_response_time, $upstream_uri, etc.) are populated when ai-proxy +sends requests via cosocket transport. + +=cut + +use t::APISIX 'no_plan'; + +log_level("info"); +repeat_each(1); +no_long_string(); +no_root_location(); + + +add_block_preprocessor(sub { + my ($block) = @_; + + if (!defined $block->request) { + $block->set_value("request", "GET /t"); + } +}); + +run_tests(); + +__DATA__ + +=== TEST 1: set route with ai-proxy pointing to mock server +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/routes/1', + ngx.HTTP_PUT, + [[{ + "uri": "/anything", + "plugins": { + "ai-proxy": { + "provider": "openai", + "auth": { + "header": { + "Authorization": "Bearer test-key" + } + }, + "options": { + "model": "gpt-4" + }, + "override": { + "endpoint": "http://127.0.0.1:1980"; + }, + "ssl_verify": false + } + } + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- response_body +passed + + + +=== TEST 2: non-streaming request populates upstream variables in access log +--- request +POST /anything +{"model":"gpt-4","messages":[{"role":"user","content":"hello"}]} +--- more_headers +X-AI-Fixture: openai/chat-basic.json +--- error_code: 200 +--- access_log eval +qr/127\.0\.0\.1:\d+ 200 [\d.]+/ + + + +=== TEST 3: streaming request populates upstream variables in access log +--- request +POST /anything +{"model":"gpt-4","messages":[{"role":"user","content":"hello"}],"stream":true} +--- more_headers +X-AI-Fixture: openai/chat-streaming.sse +--- error_code: 200 +--- access_log eval +qr/127\.0\.0\.1:\d+ 200 [\d.]+/ Review Comment: This new test file is intended to validate population of standard nginx upstream variables (eg `$upstream_response_time`, `$upstream_connect_time`, `$upstream_header_time`), but the default test `log_format` (from t::APISIX) doesn’t log those variables—only `$upstream_addr`, `$upstream_status`, and `$apisix_upstream_response_time`. As a result, TEST 2/3 currently can’t actually assert that `$upstream_response_time/connect_time/header_time` were populated. Consider overriding `access_log_format` in this test (via `--- extra_yaml_config`) to include the specific upstream vars you want to validate and then assert they are present/non-empty (and numeric). ########## docs/en/latest/plugins/ai-proxy.md: ########## @@ -2103,7 +2114,7 @@ Now if you create a Route and send a request following the [Proxy to OpenAI exam In the gateway's access log, you should see a log entry similar to the following: ```text -192.168.215.1 - - [21/Mar/2025:04:28:03 +0000] api.openai.com "POST /anything HTTP/1.1" 200 804 2.858 "-" "curl/8.6.0" - - - 5765 "http://api.openai.com"; "5c5e0b95f8d303cb81e4dc456a4b12d9" "ai_chat" "2858" "gpt-4" "gpt-4" "23" "8" +192.168.215.1 - - [21/Mar/2025:04:28:03 +0000] api.openai.com "POST /anything HTTP/1.1" 200 804 2.858 "-" "curl/8.6.0" api.openai.com:443 200 2.858 "https://api.openai.com/v1/chat/completions"; "5c5e0b95f8d303cb81e4dc456a4b12d9" "ai_chat" "2858" "gpt-4" "gpt-4" "23" "8" ``` -The access log entry shows the request type is `ai_chat`, Apisix upstream response time is `5765` milliseconds, time to first token is `2858` milliseconds, Requested LLM model is `gpt-4`. LLM model is `gpt-4`, prompt token usage is `23`, and completion token usage is `8`. +The access log entry shows the upstream address is `api.openai.com:443` with status `200`, the request type is `ai_chat`, APISIX upstream response time is `2.858` seconds, time to first token is `2858` milliseconds, requested LLM model is `gpt-4`, LLM model is `gpt-4`, prompt token usage is `23`, and completion token usage is `8`. Review Comment: The access-log example and explanation now imply `$apisix_upstream_response_time` is expressed in seconds (showing `2.858`), but in the codebase this variable is populated in milliseconds (eg `math.floor((ngx.now() - ctx.llm_request_start_time) * 1000)` in the AI provider base implementation). This makes the example log line and the narrative misleading/confusable with `$request_time` / `$upstream_response_time`. Please update the example log entry and the accompanying explanation to reflect the correct units/typical value for `$apisix_upstream_response_time`, and (optionally) clarify the unit differences between `$request_time`, `$upstream_response_time`, and `$apisix_upstream_response_time`. ########## apisix/plugins/ai-transport/http.lua: ########## @@ -71,20 +72,35 @@ end -- {method, scheme, host, port, path, headers, query, body (table), -- ssl_verify, ssl_server_name} -- @param timeout number Request timeout in milliseconds --- @return table|nil Response object (with body_reader, headers, status) +-- @return table|nil Response object (with body_reader, headers, status, +-- _upstream_addr, _upstream_uri, _connect_time, _header_time, _t0) Review Comment: The request() docstring’s “Response object” field list is out of sync with what the code actually attaches to `res`: `_upstream_host` and `_upstream_scheme` are also set (and are relied on by ai-proxy/base.lua). Please update the return-value comment so it accurately documents all attached upstream metadata fields (and their units, if relevant). ```suggestion -- _upstream_addr, _upstream_host, _upstream_scheme, _upstream_uri, -- _connect_time (seconds), _header_time (seconds), _t0) ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
