- Notifications
You must be signed in to change notification settings - Fork1.1k
feat: add AI Bridge request logs model filter#21340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Addressing feedback found in#21252> [!IMPORTANT]> This pull-request removes endpoints from `ExperimentalHandler` from`coderd.go` and promotes the endpoints within the frontend. This meansthat we will no longer be serving AI Bridge under the`/api/experimental/` prefix now that things reached release in[`v2.29.0`](https://github.com/coder/coder/releases/tag/v2.29.0).>> ### Migration >> The `/api/experimental/aibridge` prefix has been removed. Any clients,scripts, or integrations that previously called AI Bridge endpointsunder `/api/experimental/aibridge` must be updated to use the`/api/v2/aibridge` stable API routes introduced in v2.29.0.| Position | Pull-request || -------- | ------------ || | [fix: improve AI Bridge request logsUI/UX](#21252) || | [feat: add AI Bridge request logs modelfilter](#21259) || ✅ | [fix: promote AIBridge from`ExperimentalHandler`](#21278) |---------Co-authored-by: Susana Ferreira <susana@coder.com>
b2a7d9b to379b418Compare| -- Authorize Filter clause will be injected below in ListAIBridgeModelsAuthorized | ||
| -- @authorize_filter | ||
| GROUP BY | ||
| model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit: SELECT DISTINCT model could be used instead of GROUP BY model
for me it is a bit easier to read.
| returnnil,xerrors.Errorf("insert authorized filter: %w",err) | ||
| } | ||
| query:=fmt.Sprintf("-- name: ListAIBridgeModels :many\n%s",filtered) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit:-- name: ListAuthorizedAIBridgeModels
| ended_atIS NOT NULL | ||
| -- Filter model | ||
| AND CASE | ||
| WHEN @model::text!='' THENaibridge_interceptions.model ILIKE'%'|| @model::text||'%' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm a bit confused why we would want to filter model by model name? There shouldn't be that many unique models? Could we return all models and filter in the frontend (in doogfood there are distinct 28 models now)?
Problem with filtering by text in backend is efficiency,aibridge_interceptions table can be quite large.
If I understand correctly, current code will do full table scan.
To use existing B-tree index onmodel column filtering should be case sensitive prefix matching:aibridge_interceptions.model LIKE @model::text || '%'
or different index needs to be created:https://www.postgresql.org/docs/current/pgtrgm.html#PGTRGM-INDEX
Also if I understand correctly authorization will add some WHERE initiator_id = "..." clause. There is no composite index but I don't think it matters that much in this case as there are single column indexes on both initiator_id and model columns.
| // TODO: remove with Beta release. | ||
| r.Route("/aibridge",aibridgeHandler(api,apiKeyMiddleware)) | ||
| api.AGPL.ExperimentalHandler.Group(func(_ chi.Router) { | ||
| // Add enterprise-only experimental routes here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't like removing experimental paths here. It should be separate issue / PR.
Recreation of#21259
This implements a simple filter for querying against AI Bridge with a given model to the frontend UI. With a simple set of backend changes to return back the models which are filterable against.
Furthermore, I made a few changes to ensure that the imports of
/filterare more understandable/scoped, they now live underRequestLogsFilter.Preview
aibridge-model-filter.mp4
ExperimentalHandler<RequestLogsPrompt />)<SyntaxHighlighter />for AI BridgeToken Usages Metadata@codex