-
Notifications
You must be signed in to change notification settings - Fork 1
feat: runtime-configurable upstream request logging #37
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 our terms of service and privacy 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?
Changes from all commits
785748c
a8b8db2
0452dc8
a4c07af
7a3c4bf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,60 @@ | ||
| package aibridge | ||
|
|
||
| import "go.uber.org/atomic" | ||
|
|
||
| type ProviderConfig struct { | ||
| BaseURL, Key string | ||
| baseURL, key atomic.String | ||
| upstreamLoggingDir atomic.String | ||
dannykopping marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| enableUpstreamLogging atomic.Bool | ||
|
Comment on lines
+7
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need to be two values? An empty string could denote that it's disabled There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An empty |
||
| } | ||
|
|
||
| // NewProviderConfig creates a new ProviderConfig with the given values. | ||
| func NewProviderConfig(baseURL, key, upstreamLoggingDir string) *ProviderConfig { | ||
| cfg := &ProviderConfig{} | ||
| cfg.baseURL.Store(baseURL) | ||
| cfg.key.Store(key) | ||
| cfg.upstreamLoggingDir.Store(upstreamLoggingDir) | ||
| return cfg | ||
| } | ||
|
|
||
| // BaseURL returns the base URL for the provider. | ||
| func (c *ProviderConfig) BaseURL() string { | ||
| return c.baseURL.Load() | ||
| } | ||
|
|
||
| // SetBaseURL sets the base URL for the provider. | ||
| func (c *ProviderConfig) SetBaseURL(baseURL string) { | ||
| c.baseURL.Store(baseURL) | ||
| } | ||
|
|
||
| // Key returns the API key for the provider. | ||
| func (c *ProviderConfig) Key() string { | ||
| return c.key.Load() | ||
| } | ||
|
|
||
| // SetKey sets the API key for the provider. | ||
| func (c *ProviderConfig) SetKey(key string) { | ||
| c.key.Store(key) | ||
| } | ||
|
|
||
| // UpstreamLoggingDir returns the base directory for upstream logging. | ||
| // If empty, the OS's tempdir will be used. | ||
| // Logs are written to $UpstreamLoggingDir/$provider/$model/$interceptionID.{req,res}.log | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. File per interception seems like a lot of files. For short debugging it should be fine but not in general. Maybe instead of directory structure / separate files structured logging could work? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Structured logging won't be easy to read because of the large payloads. |
||
| func (c *ProviderConfig) UpstreamLoggingDir() string { | ||
| return c.upstreamLoggingDir.Load() | ||
| } | ||
|
|
||
| // SetUpstreamLoggingDir sets the base directory for upstream logging. | ||
| func (c *ProviderConfig) SetUpstreamLoggingDir(dir string) { | ||
| c.upstreamLoggingDir.Store(dir) | ||
| } | ||
|
|
||
| // SetEnableUpstreamLogging enables or disables upstream logging at runtime. | ||
| func (c *ProviderConfig) SetEnableUpstreamLogging(enabled bool) { | ||
| c.enableUpstreamLogging.Store(enabled) | ||
| } | ||
|
|
||
| type Config struct { | ||
| OpenAI ProviderConfig | ||
| Anthropic ProviderConfig | ||
| // IsUpstreamLoggingEnabled returns whether upstream logging is currently enabled. | ||
| func (c *ProviderConfig) IsUpstreamLoggingEnabled() bool { | ||
| return c.enableUpstreamLogging.Load() | ||
| } | ||
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.
This is a lot especially given that the baseURL and key won't change during a provider's lifetime within Coder.
Have you considered moving the methods for changing upstream logging settings to the provider rather than having the config be mutable? In Coder for example, we don't change the
coderd.Optionstype after passing it to the API.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'll have a look.