feat: add performance metrics and monitoring system#211
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Summary by CodeRabbit
WalkthroughThis update introduces comprehensive performance and usage metrics throughout the Bifrost system and its OpenAI provider. It adds timing and pool usage tracking, embeds metrics in responses, and implements a mock response for OpenAI chat completions. The HTTP transport now supports graceful shutdown, and logging plugin structures are slightly refactored for tool data handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Bifrost
participant Provider (OpenAI)
participant Metrics
Client->>Bifrost: Send request
Bifrost->>Bifrost: Record enqueue timestamp
Bifrost->>Provider (OpenAI): Forward request
Provider (OpenAI)->>Provider (OpenAI): Generate mock response and timings
Provider (OpenAI)->>Bifrost: Return response + metrics
Bifrost->>Metrics: Record timings and pool usage (async)
Bifrost->>Client: Return response with embedded metrics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
core/providers/openai.go (1)
228-241: RawResponse field is overwrittenThe
RawResponsefield is set twice - first with the actual response data (line 230) and then overwritten with just the timings (lines 239-241). This loses the original response data whensendBackRawResponseis enabled.// Set raw response if enabled if provider.sendBackRawResponse { response.ExtraFields.RawResponse = rawResponse } if params != nil { response.ExtraFields.Params = *params } timings["total_response_parsing"] = time.Since(parseStart) - response.ExtraFields.RawResponse = map[string]interface{}{ - "timings": timings, - } + // Add timings to the response + if response.ExtraFields.RawResponse == nil { + response.ExtraFields.RawResponse = make(map[string]interface{}) + } + if rawMap, ok := response.ExtraFields.RawResponse.(map[string]interface{}); ok { + rawMap["timings"] = timings + } else { + // If RawResponse is not a map, create a wrapper + response.ExtraFields.RawResponse = map[string]interface{}{ + "data": response.ExtraFields.RawResponse, + "timings": timings, + } + }
| func (bifrost *Bifrost) recordError(queueWaitTime, keySelectTime, providerTime, pluginPreTime, pluginPostTime time.Duration) { | ||
| // Spawn goroutine to avoid blocking the request path | ||
| go func() { | ||
| atomic.AddInt64(&bifrost.metrics.RequestCount, 1) | ||
| atomic.AddInt64(&bifrost.metrics.ErrorCount, 1) | ||
|
|
||
| totalTime := queueWaitTime + keySelectTime + providerTime + pluginPreTime + pluginPostTime | ||
|
|
||
| // Add to accumulators | ||
| atomic.AddInt64(&bifrost.metrics.TotalTimeNs, totalTime.Nanoseconds()) | ||
| atomic.AddInt64(&bifrost.metrics.QueueWaitTimeNs, queueWaitTime.Nanoseconds()) | ||
| atomic.AddInt64(&bifrost.metrics.KeySelectionTimeNs, keySelectTime.Nanoseconds()) | ||
| atomic.AddInt64(&bifrost.metrics.ProviderTimeNs, providerTime.Nanoseconds()) | ||
| atomic.AddInt64(&bifrost.metrics.PluginPreTimeNs, pluginPreTime.Nanoseconds()) | ||
| atomic.AddInt64(&bifrost.metrics.PluginPostTimeNs, pluginPostTime.Nanoseconds()) | ||
| }() | ||
| } | ||
|
|
||
| func (bifrost *Bifrost) recordMetrics(queueWaitTime, keySelectTime, providerTime, pluginPreTime, pluginPostTime, totalTime time.Duration, success bool) { | ||
| // Spawn goroutine to avoid blocking the request path | ||
| go func() { | ||
| atomic.AddInt64(&bifrost.metrics.RequestCount, 1) | ||
| if !success { | ||
| atomic.AddInt64(&bifrost.metrics.ErrorCount, 1) | ||
| } | ||
|
|
||
| // Add to accumulators | ||
| atomic.AddInt64(&bifrost.metrics.TotalTimeNs, totalTime.Nanoseconds()) | ||
| atomic.AddInt64(&bifrost.metrics.QueueWaitTimeNs, queueWaitTime.Nanoseconds()) | ||
| atomic.AddInt64(&bifrost.metrics.KeySelectionTimeNs, keySelectTime.Nanoseconds()) | ||
| atomic.AddInt64(&bifrost.metrics.ProviderTimeNs, providerTime.Nanoseconds()) | ||
| atomic.AddInt64(&bifrost.metrics.PluginPreTimeNs, pluginPreTime.Nanoseconds()) | ||
| atomic.AddInt64(&bifrost.metrics.PluginPostTimeNs, pluginPostTime.Nanoseconds()) | ||
| }() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to reduce code duplication between recordError and recordMetrics.
The recordError method duplicates logic from recordMetrics. Consider refactoring to reuse code.
Apply this refactor:
-func (bifrost *Bifrost) recordError(queueWaitTime, keySelectTime, providerTime, pluginPreTime, pluginPostTime time.Duration) {
- // Spawn goroutine to avoid blocking the request path
- go func() {
- atomic.AddInt64(&bifrost.metrics.RequestCount, 1)
- atomic.AddInt64(&bifrost.metrics.ErrorCount, 1)
-
- totalTime := queueWaitTime + keySelectTime + providerTime + pluginPreTime + pluginPostTime
-
- // Add to accumulators
- atomic.AddInt64(&bifrost.metrics.TotalTimeNs, totalTime.Nanoseconds())
- atomic.AddInt64(&bifrost.metrics.QueueWaitTimeNs, queueWaitTime.Nanoseconds())
- atomic.AddInt64(&bifrost.metrics.KeySelectionTimeNs, keySelectTime.Nanoseconds())
- atomic.AddInt64(&bifrost.metrics.ProviderTimeNs, providerTime.Nanoseconds())
- atomic.AddInt64(&bifrost.metrics.PluginPreTimeNs, pluginPreTime.Nanoseconds())
- atomic.AddInt64(&bifrost.metrics.PluginPostTimeNs, pluginPostTime.Nanoseconds())
- }()
+func (bifrost *Bifrost) recordError(queueWaitTime, keySelectTime, providerTime, pluginPreTime, pluginPostTime time.Duration) {
+ totalTime := queueWaitTime + keySelectTime + providerTime + pluginPreTime + pluginPostTime
+ bifrost.recordMetrics(queueWaitTime, keySelectTime, providerTime, pluginPreTime, pluginPostTime, totalTime, false)
}🤖 Prompt for AI Agents
In core/bifrost.go around lines 1636 to 1670, the recordError method duplicates
much of the logic found in recordMetrics. Refactor by modifying recordError to
call recordMetrics with the appropriate parameters, setting success to false and
computing totalTime as the sum of the individual durations. This will eliminate
code duplication and centralize metric recording logic in recordMetrics.
| // Add current runtime memory statistics | ||
| var currentMem runtime.MemStats | ||
| runtime.ReadMemStats(¤tMem) |
There was a problem hiding this comment.
Consider the performance impact of runtime.ReadMemStats.
runtime.ReadMemStats is a stop-the-world operation that pauses all goroutines. Calling it frequently (e.g., on every API response) could impact latency.
Consider these alternatives:
- Cache the memory stats and update them periodically in a background goroutine
- Make memory stats collection optional via a parameter
- Use lighter-weight alternatives like
runtime.MemStats.Allocdirectly
Example implementation with caching:
// Add to Bifrost struct
memStatsCache struct {
sync.RWMutex
stats runtime.MemStats
lastUpdate time.Time
}
// Update periodically in background
func (bifrost *Bifrost) updateMemStatsCache() {
ticker := time.NewTicker(5 * time.Second)
go func() {
for range ticker.C {
var stats runtime.MemStats
runtime.ReadMemStats(&stats)
bifrost.memStatsCache.Lock()
bifrost.memStatsCache.stats = stats
bifrost.memStatsCache.lastUpdate = time.Now()
bifrost.memStatsCache.Unlock()
}
}()
}🤖 Prompt for AI Agents
In core/bifrost.go around lines 1708 to 1710, runtime.ReadMemStats is called
directly, which is a stop-the-world operation and can degrade performance if
called frequently. To fix this, implement a caching mechanism by adding a
memStatsCache struct with a mutex and timestamp to the Bifrost struct, then
start a background goroutine that updates this cache periodically (e.g., every 5
seconds) using runtime.ReadMemStats. Replace direct calls to
runtime.ReadMemStats with reads from the cached stats under a read lock to
minimize latency impact.
| // Check if OpenAI provider exists and add its metrics | ||
| if providerInstance, exists := bifrost.providerInstances.Load(schemas.OpenAI); exists { | ||
| if openAIProvider, ok := providerInstance.(*providers.OpenAIProvider); ok { | ||
| stats["provider_metrics"].(map[string]interface{})["openai"] = openAIProvider.GetOpenAIMetrics() | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider making provider metrics collection more extensible.
Currently, only OpenAI provider metrics are collected. Consider defining a MetricsProvider interface to make this extensible to other providers.
Example approach:
// Define interface
type MetricsProvider interface {
GetProviderMetrics() map[string]interface{}
}
// In GetAllStats
bifrost.providerInstances.Range(func(key, value interface{}) bool {
if metricsProvider, ok := value.(MetricsProvider); ok {
providerKey := key.(schemas.ModelProvider)
stats["provider_metrics"].(map[string]interface{})[string(providerKey)] = metricsProvider.GetProviderMetrics()
}
return true
})🤖 Prompt for AI Agents
In core/bifrost.go around lines 1742 to 1747, the current code only collects
metrics for the OpenAI provider, limiting extensibility. To fix this, define a
MetricsProvider interface with a GetProviderMetrics method, then iterate over
all providerInstances using Range, checking if each implements MetricsProvider.
For those that do, call GetProviderMetrics and add the results to the
stats["provider_metrics"] map keyed by the provider name. This will generalize
metrics collection for all providers.
| New: func() interface{} { | ||
| openAIPoolGets.Add(1) | ||
| return &schemas.BifrostResponse{} | ||
| }, | ||
| } |
There was a problem hiding this comment.
Fix incorrect pool metrics tracking in New function
The pool's New function should increment a creation counter, not the get counter. Currently, it incorrectly tracks object creation as a "get" operation.
openAIResponsePool = sync.Pool{
New: func() interface{} {
- openAIPoolGets.Add(1)
+ openAIPoolCreations.Add(1)
return &schemas.BifrostResponse{}
},
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In core/providers/openai.go around lines 48 to 52, the New function incorrectly
increments the openAIPoolGets counter, which tracks gets instead of creations.
Change the increment to the correct creation counter, such as openAIPoolCreates
or a similarly named metric that tracks object creation, to accurately reflect
the pool's creation events.
| timings := make(map[string]time.Duration) | ||
|
|
||
| formattedMessages, preparedParams := prepareOpenAIChatRequest(messages, params) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add message preparation timing
The metrics recording expects total_message_preparation timing, but it's not being tracked in the ChatCompletion method.
timings := make(map[string]time.Duration)
+ // Track message preparation time
+ messagePrepStart := time.Now()
formattedMessages, preparedParams := prepareOpenAIChatRequest(messages, params)
+ timings["total_message_preparation"] = time.Since(messagePrepStart)🤖 Prompt for AI Agents
In core/providers/openai.go around lines 136 to 139, the code calls
prepareOpenAIChatRequest but does not measure or record the time taken for
message preparation. To fix this, add timing code around the
prepareOpenAIChatRequest call to measure its duration, then store this duration
in the timings map under the key "total_message_preparation" so that the metric
is properly tracked.
| mockResponse := mockOpenAIChatCompletionResponse(req, model) | ||
| // Copy the mock response body to the real response | ||
| resp.SetBody(mockResponse) | ||
| // Simulate network delay | ||
| jitter := time.Duration(float64(1500*time.Millisecond) * (0.6 + 0.8*rand.Float64())) | ||
| time.Sleep(jitter) | ||
|
|
||
| // Make request | ||
| bifrostErr := makeRequestWithContext(ctx, provider.client, req, resp) | ||
| if bifrostErr != nil { | ||
| return nil, bifrostErr | ||
| } | ||
| // bifrostErr := makeRequestWithContext(ctx, provider.client, req, resp) | ||
| timings["http_request"] = time.Since(httpStart) | ||
| // if bifrostErr != nil { | ||
| // provider.recordOpenAIMetrics(timings, false) // Record metrics for failed request | ||
| // return nil, bifrostErr | ||
| // } |
There was a problem hiding this comment.
Critical: Production code replaced with mock response
The actual HTTP request to OpenAI has been commented out and replaced with a mock response. This completely breaks the provider's functionality in production.
- mockResponse := mockOpenAIChatCompletionResponse(req, model)
- // Copy the mock response body to the real response
- resp.SetBody(mockResponse)
- // Simulate network delay
- jitter := time.Duration(float64(1500*time.Millisecond) * (0.6 + 0.8*rand.Float64()))
- time.Sleep(jitter)
-
// Make request
- // bifrostErr := makeRequestWithContext(ctx, provider.client, req, resp)
+ bifrostErr := makeRequestWithContext(ctx, provider.client, req, resp)
timings["http_request"] = time.Since(httpStart)
- // if bifrostErr != nil {
- // provider.recordOpenAIMetrics(timings, false) // Record metrics for failed request
- // return nil, bifrostErr
- // }
+ if bifrostErr != nil {
+ provider.recordOpenAIMetrics(timings, false) // Record metrics for failed request
+ return nil, bifrostErr
+ }🤖 Prompt for AI Agents
In core/providers/openai.go between lines 179 and 192, the real HTTP request to
OpenAI is commented out and replaced with a mock response, which breaks
production functionality. Restore the original code by uncommenting the actual
HTTP request call and its error handling, and remove the mock response and
simulated delay to ensure the provider works correctly in production.
| // OpenAIMetrics stores provider-specific metrics using atomic operations for high-throughput scenarios | ||
| type OpenAIMetrics struct { | ||
| // Counters | ||
| RequestCount int64 | ||
| ErrorCount int64 | ||
|
|
||
| // Time accumulators (in nanoseconds for atomic operations) | ||
| TotalTimeNs int64 | ||
| MessagePreparationTimeNs int64 | ||
| RequestBodyPreparationTimeNs int64 | ||
| JSONMarshalingTimeNs int64 | ||
| RequestSetupTimeNs int64 | ||
| HTTPRequestTimeNs int64 | ||
| ErrorHandlingTimeNs int64 | ||
| PoolAcquisitionTimeNs int64 | ||
| JSONUnmarshalingTimeNs int64 | ||
| ResponseParsingTimeNs int64 | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using atomic types for metrics fields
While the current implementation using atomic.AddInt64 and atomic.LoadInt64 is correct, consider using Go's atomic types for better type safety and cleaner code.
type OpenAIMetrics struct {
// Counters
- RequestCount int64
- ErrorCount int64
+ RequestCount atomic.Int64
+ ErrorCount atomic.Int64
// Time accumulators (in nanoseconds for atomic operations)
- TotalTimeNs int64
- MessagePreparationTimeNs int64
- RequestBodyPreparationTimeNs int64
- JSONMarshalingTimeNs int64
- RequestSetupTimeNs int64
- HTTPRequestTimeNs int64
- ErrorHandlingTimeNs int64
- PoolAcquisitionTimeNs int64
- JSONUnmarshalingTimeNs int64
- ResponseParsingTimeNs int64
+ TotalTimeNs atomic.Int64
+ MessagePreparationTimeNs atomic.Int64
+ RequestBodyPreparationTimeNs atomic.Int64
+ JSONMarshalingTimeNs atomic.Int64
+ RequestSetupTimeNs atomic.Int64
+ HTTPRequestTimeNs atomic.Int64
+ ErrorHandlingTimeNs atomic.Int64
+ PoolAcquisitionTimeNs atomic.Int64
+ JSONUnmarshalingTimeNs atomic.Int64
+ ResponseParsingTimeNs atomic.Int64
}Then update the usage to:
provider.metrics.RequestCount.Add(1)
requestCount := provider.metrics.RequestCount.Load()🤖 Prompt for AI Agents
In core/providers/openai.go around lines 1208 to 1225, the OpenAIMetrics struct
uses int64 fields updated with atomic functions, but it should use Go's atomic
types like atomic.Int64 for better type safety and cleaner code. Change each
int64 field to atomic.Int64 type and update all increments and loads to use the
Add and Load methods on these atomic.Int64 fields accordingly.
| // mockOpenAIResponse creates a mock response for OpenAI API calls | ||
| func mockOpenAIChatCompletionResponse(req *fasthttp.Request, model string) []byte { | ||
| // Create a mock response that mimics OpenAI's format | ||
| mockResp := &OpenAIResponse{ | ||
| ID: "mock-" + model + "-" + fmt.Sprintf("%d", time.Now().Unix()), | ||
| Object: "chat.completion", | ||
| Model: model, | ||
| Created: int(time.Now().Unix()), | ||
| Choices: []schemas.BifrostResponseChoice{ | ||
| { | ||
| Index: 0, | ||
| BifrostNonStreamResponseChoice: &schemas.BifrostNonStreamResponseChoice{ | ||
| Message: schemas.BifrostMessage{ | ||
| Role: schemas.ModelChatMessageRoleAssistant, | ||
| Content: schemas.MessageContent{ | ||
| ContentStr: StrPtr("This is a mock response from the Bifrost API gateway. The actual API was not called. " + | ||
| "This response has been expanded to demonstrate the system's ability to handle larger payloads. " + | ||
| "In a real-world scenario, this could be a comprehensive analysis, detailed explanation, or extensive documentation. " + | ||
| "The Bifrost API gateway serves as a unified interface for multiple AI providers, offering seamless integration " + | ||
| "and consistent response formats across different language models and services. " + | ||
| "When operating in mock mode, the system generates realistic responses that mirror the expected output " + | ||
| "from actual AI providers while maintaining the same structure and formatting conventions. " + | ||
| "This capability is particularly useful for testing, development, and demonstration purposes " + | ||
| "where you need predictable responses without consuming actual API credits or making real network calls. " + | ||
| "The mock responses can be configured to simulate various scenarios including success cases, " + | ||
| "error conditions, streaming responses, and different content types such as text, code, and structured data. " + | ||
| "Additionally, the system supports comprehensive logging and monitoring of all interactions, " + | ||
| "providing detailed insights into request patterns, response times, token usage, and system performance metrics. " + | ||
| "This mock response continues with additional content to reach the desired payload size of 10KB. " + | ||
| "Performance optimization is a key consideration in the design of this system, ensuring that " + | ||
| "large responses can be handled efficiently without compromising system stability or user experience. " + | ||
| "The architecture supports horizontal scaling, load balancing, and fault tolerance mechanisms " + | ||
| "to maintain high availability and reliability even under heavy load conditions. " + | ||
| "Security features include authentication, authorization, rate limiting, and comprehensive audit logging " + | ||
| "to ensure that all API interactions are properly tracked and controlled. " + | ||
| "The system also provides extensive configuration options allowing administrators to customize " + | ||
| "behavior based on specific requirements and use cases. " + | ||
| "Documentation and examples are provided to help developers integrate with the API quickly and effectively. " + | ||
| "The mock mode serves as an excellent starting point for understanding the API structure and response formats " + | ||
| "before moving to production deployments with actual AI provider integrations. " + | ||
| "This extended mock response demonstrates the system's capability to handle substantial content volumes " + | ||
| "while maintaining proper JSON formatting and response structure compliance. " + | ||
| "The implementation includes proper error handling, timeout management, and resource cleanup " + | ||
| "to ensure robust operation in production environments. " + | ||
| "Monitoring and alerting capabilities provide real-time visibility into system health and performance, " + | ||
| "enabling proactive identification and resolution of potential issues. " + | ||
| "The API supports various content types including plain text, markdown, HTML, JSON, XML, and binary data, " + | ||
| "making it suitable for diverse application requirements and integration scenarios. " + | ||
| "Advanced features include request transformation, response filtering, content validation, " + | ||
| "and custom middleware support for implementing specialized business logic. " + | ||
| "The system is designed to be provider-agnostic, allowing seamless switching between different AI services " + | ||
| "without requiring changes to client applications or integration code. " + | ||
| "This flexibility enables organizations to optimize costs, performance, and capabilities " + | ||
| "by leveraging the best features of multiple AI providers through a single, unified interface. " + | ||
| "Comprehensive testing suites ensure reliability and compatibility across all supported providers and features. " + | ||
| "The mock response system includes sophisticated simulation capabilities that can replicate " + | ||
| "real-world usage patterns and edge cases to support thorough testing and validation processes. " + | ||
| "Performance benchmarking tools are integrated to measure and optimize system throughput, latency, " + | ||
| "and resource utilization under various load conditions and configuration settings. " + | ||
| "The architecture supports both synchronous and asynchronous processing models, " + | ||
| "enabling efficient handling of both real-time interactions and batch processing scenarios. " + | ||
| "Data persistence and caching mechanisms are implemented to improve response times " + | ||
| "and reduce external API calls when appropriate, while maintaining data freshness and accuracy. " + | ||
| "The system includes comprehensive logging and analytics capabilities that provide insights " + | ||
| "into usage patterns, performance trends, and potential optimization opportunities. " + | ||
| "This mock response continues to provide additional content to demonstrate the system's ability " + | ||
| "to handle large payloads efficiently and effectively while maintaining proper formatting and structure. " + | ||
| "The implementation follows industry best practices for API design, security, and performance, " + | ||
| "ensuring compatibility with existing development workflows and deployment processes. " + | ||
| "Support for multiple programming languages and frameworks makes integration straightforward " + | ||
| "regardless of the technology stack used by client applications. " + | ||
| "The system provides detailed documentation, code examples, and interactive tutorials " + | ||
| "to help developers get started quickly and implement advanced features effectively. " + | ||
| "This comprehensive mock response serves as an example of the type of detailed, " + | ||
| "informative content that can be processed and delivered through the Bifrost API gateway, " + | ||
| "demonstrating its capability to handle substantial payloads while maintaining high performance " + | ||
| "The architecture supports both synchronous and asynchronous processing models, " + | ||
| "enabling efficient handling of both real-time interactions and batch processing scenarios. " + | ||
| "Data persistence and caching mechanisms are implemented to improve response times " + | ||
| "and reduce external API calls when appropriate, while maintaining data freshness and accuracy. " + | ||
| "The system includes comprehensive logging and analytics capabilities that provide insights " + | ||
| "into usage patterns, performance trends, and potential optimization opportunities. " + | ||
| "This mock response continues to provide additional content to demonstrate the system's ability " + | ||
| "to handle large payloads efficiently and effectively while maintaining proper formatting and structure. " + | ||
| "The implementation follows industry best practices for API design, security, and performance, " + | ||
| "ensuring compatibility with existing development workflows and deployment processes. " + | ||
| "Support for multiple programming languages and frameworks makes integration straightforward " + | ||
| "regardless of the technology stack used by client applications. " + | ||
| "The system provides detailed documentation, code examples, and interactive tutorials " + | ||
| "to help developers get started quickly and implement advanced features effectively. " + | ||
| "This comprehensive mock response serves as an example of the type of detailed, " + | ||
| "informative content that can be processed and delivered through the Bifrost API gateway, " + | ||
| "demonstrating its capability to handle substantial payloads while maintaining high performance " + | ||
| "The architecture supports both synchronous and asynchronous processing models, " + | ||
| "enabling efficient handling of both real-time interactions and batch processing scenarios. " + | ||
| "Data persistence and caching mechanisms are implemented to improve response times " + | ||
| "and reduce external API calls when appropriate, while maintaining data freshness and accuracy. " + | ||
| "The system includes comprehensive logging and analytics capabilities that provide insights " + | ||
| "into usage patterns, performance trends, and potential optimization opportunities. " + | ||
| "This mock response continues to provide additional content to demonstrate the system's ability " + | ||
| "to handle large payloads efficiently and effectively while maintaining proper formatting and structure. " + | ||
| "The implementation follows industry best practices for API design, security, and performance, " + | ||
| "ensuring compatibility with existing development workflows and deployment processes. " + | ||
| "Support for multiple programming languages and frameworks makes integration straightforward " + | ||
| "regardless of the technology stack used by client applications. " + | ||
| "The system provides detailed documentation, code examples, and interactive tutorials " + | ||
| "to help developers get started quickly and implement advanced features effectively. " + | ||
| "This comprehensive mock response serves as an example of the type of detailed, " + | ||
| "informative content that can be processed and delivered through the Bifrost API gateway, " + | ||
| "demonstrating its capability to handle substantial payloads while maintaining high performance " + | ||
| "The architecture supports both synchronous and asynchronous processing models, " + | ||
| "enabling efficient handling of both real-time interactions and batch processing scenarios. " + | ||
| "Data persistence and caching mechanisms are implemented to improve response times " + | ||
| "and reduce external API calls when appropriate, while maintaining data freshness and accuracy. " + | ||
| "The system includes comprehensive logging and analytics capabilities that provide insights " + | ||
| "into usage patterns, performance trends, and potential optimization opportunities. " + | ||
| "This mock response continues to provide additional content to demonstrate the system's ability " + | ||
| "to handle large payloads efficiently and effectively while maintaining proper formatting and structure. " + | ||
| "The implementation follows industry best practices for API design, security, and performance, " + | ||
| "ensuring compatibility with existing development workflows and deployment processes. " + | ||
| "Support for multiple programming languages and frameworks makes integration straightforward " + | ||
| "and reliability standards expected in production environments."), | ||
| }, | ||
| }, | ||
| }, | ||
| FinishReason: StrPtr("stop"), | ||
| }, | ||
| }, | ||
| Usage: schemas.LLMUsage{ | ||
| PromptTokens: 100, | ||
| CompletionTokens: 50, | ||
| TotalTokens: 150, | ||
| }, | ||
| } | ||
|
|
||
| // Convert to JSON | ||
| mockJSON, err := json.Marshal(mockResp) | ||
| if err != nil { | ||
| return nil | ||
| } | ||
|
|
||
| return mockJSON | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove mock response function from production code
This mock response function should not be in production code. Mocking should be handled in tests or through configuration/feature flags.
Additionally:
- Uses
json.Marshalinstead ofsonic.Marshal(inconsistent with the rest of the codebase) - References undefined
StrPtrfunction - Contains excessive repetitive text
Consider:
- Moving this to a test file or test utilities package
- If mocking is needed in production, implement it via configuration/feature flags
- Use
sonic.Marshalfor consistency - Define or import the
StrPtrhelper function
🤖 Prompt for AI Agents
In core/providers/openai.go between lines 1348 and 1490, the
mockOpenAIChatCompletionResponse function should be removed from production code
as mocking belongs in tests or controlled by feature flags. Move this function
to a test file or test utilities package. Replace json.Marshal with
sonic.Marshal for consistency with the codebase. Define or import the StrPtr
helper function used for string pointers. Also, remove the excessive repetitive
text in the mock response content to keep it concise and maintainable.
Pratham-Mishra04
force-pushed
the
07-29-dev_performance_tracking_for_benchmarking
branch
from
aa79887 to
a240201
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
core/bifrost.go (1)
1201-1268: Include retry wait time in metrics or remove unused variable.The
totalRetryWaitTimevariable is calculated but never used in the metrics recording. Either include it in the metrics or remove the unused tracking.Consider adding retry metrics to the
recordMetricscall or create a separate retry metrics field:-bifrost.recordMetrics(queueWaitTime, keySelectTime, providerTime, 0, 0, totalTime, bifrostError == nil) +// Include retry information in metrics if needed +bifrost.recordMetrics(queueWaitTime, keySelectTime, providerTime, 0, 0, totalTime, bifrostError == nil) +// TODO: Add totalRetryWaitTime to metrics structure if retry timing is important
♻️ Duplicate comments (8)
core/providers/openai.go (5)
1208-1225: Consider using atomic types for metrics fieldsWhile the current implementation using
atomic.AddInt64andatomic.LoadInt64is correct, consider using Go's atomic types for better type safety and cleaner code.type OpenAIMetrics struct { // Counters - RequestCount int64 - ErrorCount int64 + RequestCount atomic.Int64 + ErrorCount atomic.Int64 // Time accumulators (in nanoseconds for atomic operations) - TotalTimeNs int64 - MessagePreparationTimeNs int64 - RequestBodyPreparationTimeNs int64 - JSONMarshalingTimeNs int64 - RequestSetupTimeNs int64 - HTTPRequestTimeNs int64 - ErrorHandlingTimeNs int64 - PoolAcquisitionTimeNs int64 - JSONUnmarshalingTimeNs int64 - ResponseParsingTimeNs int64 + TotalTimeNs atomic.Int64 + MessagePreparationTimeNs atomic.Int64 + RequestBodyPreparationTimeNs atomic.Int64 + JSONMarshalingTimeNs atomic.Int64 + RequestSetupTimeNs atomic.Int64 + HTTPRequestTimeNs atomic.Int64 + ErrorHandlingTimeNs atomic.Int64 + PoolAcquisitionTimeNs atomic.Int64 + JSONUnmarshalingTimeNs atomic.Int64 + ResponseParsingTimeNs atomic.Int64 }Then update the usage to:
provider.metrics.RequestCount.Add(1) requestCount := provider.metrics.RequestCount.Load()
48-52: Fix incorrect pool metrics tracking in New functionThe pool's
Newfunction should increment the creation counter, not the get counter. This will cause incorrect pool efficiency metrics.openAIResponsePool = sync.Pool{ New: func() interface{} { - openAIPoolGets.Add(1) + openAIPoolCreations.Add(1) return &schemas.BifrostResponse{} }, }
1348-1490: Remove mock response function from production codeThis mock response function should not be in production code. Additionally:
- Uses
json.Marshalinstead ofsonic.Marshal(inconsistent with the rest of the codebase)- References undefined
StrPtrfunction- Contains excessive repetitive text
Consider:
- Moving this to a test file or test utilities package
- If mocking is needed in production, implement it via configuration/feature flags
- Use
sonic.Marshalfor consistency- Define or import the
StrPtrhelper function-// mockOpenAIChatCompletionResponse creates a mock response for OpenAI API calls -func mockOpenAIChatCompletionResponse(req *fasthttp.Request, model string) []byte { - // ... entire function ... -}Move to a test utilities file or remove entirely from production code.
136-139: Add message preparation timingThe metrics recording expects
total_message_preparationtiming, but it's not being tracked in the ChatCompletion method.timings := make(map[string]time.Duration) + // Track message preparation time + messagePrepStart := time.Now() formattedMessages, preparedParams := prepareOpenAIChatRequest(messages, params) + timings["total_message_preparation"] = time.Since(messagePrepStart)
179-192: Critical: Production code replaced with mock responseThe actual HTTP request to OpenAI has been commented out and replaced with a mock response. This completely breaks the provider's functionality in production.
- mockResponse := mockOpenAIChatCompletionResponse(req, model) - // Copy the mock response body to the real response - resp.SetBody(mockResponse) - // Simulate network delay - jitter := time.Duration(float64(1500*time.Millisecond) * (0.6 + 0.8*rand.Float64())) - time.Sleep(jitter) - // Make request - // bifrostErr := makeRequestWithContext(ctx, provider.client, req, resp) + bifrostErr := makeRequestWithContext(ctx, provider.client, req, resp) timings["http_request"] = time.Since(httpStart) - // if bifrostErr != nil { - // provider.recordOpenAIMetrics(timings, false) // Record metrics for failed request - // return nil, bifrostErr - // } + if bifrostErr != nil { + provider.recordOpenAIMetrics(timings, false) // Record metrics for failed request + return nil, bifrostErr + }core/bifrost.go (3)
1639-1673: Refactor to reduce code duplication between recordError and recordMetrics.The
recordErrormethod duplicates logic fromrecordMetrics. Consider refactoring to reuse code.
1711-1713: Consider the performance impact of runtime.ReadMemStats.
runtime.ReadMemStatsis a stop-the-world operation that pauses all goroutines. Calling it frequently (e.g., on every API response) could impact latency.
1745-1750: Consider making provider metrics collection more extensible.Currently, only OpenAI provider metrics are collected. Consider defining a
MetricsProviderinterface to make this extensible to other providers.
| // Returns an error if initialization fails. | ||
| // Initial Memory Allocations happens here as per the initial pool size. | ||
| func Init(config schemas.BifrostConfig) (*Bifrost, error) { | ||
| debug.SetGCPercent(-1) |
There was a problem hiding this comment.
Critical: Do not disable garbage collection in production code.
Calling debug.SetGCPercent(-1) completely disables Go's garbage collector, which will cause memory to never be freed and lead to eventual out-of-memory crashes. This is dangerous for any long-running service.
If you need to tune GC for performance, consider using a higher percentage (e.g., debug.SetGCPercent(200)) instead of disabling it entirely. Alternatively, use the GOGC environment variable to allow runtime configuration.
🤖 Prompt for AI Agents
In core/bifrost.go at line 117, the code disables garbage collection by calling
debug.SetGCPercent(-1), which can cause memory leaks and crashes in production.
To fix this, remove or replace this call with a safer tuning value like
debug.SetGCPercent(200) or rely on the GOGC environment variable for runtime GC
configuration, ensuring garbage collection remains enabled.
| response.ExtraFields.RawResponse = map[string]interface{}{ | ||
| "timings": timings, | ||
| } | ||
|
|
There was a problem hiding this comment.
Don't overwrite RawResponse with timing data
The code overwrites the RawResponse field with timing data, which breaks the contract when sendBackRawResponse is enabled. Timing data should be stored separately.
- response.ExtraFields.RawResponse = map[string]interface{}{
- "timings": timings,
- }
+ // Store timings in a separate field or merge with existing RawResponse
+ if response.ExtraFields.RawResponse != nil {
+ // If RawResponse exists, add timings to it
+ if rawMap, ok := response.ExtraFields.RawResponse.(map[string]interface{}); ok {
+ rawMap["_timings"] = timings
+ }
+ } else {
+ // Store timings separately if no RawResponse
+ response.ExtraFields.Timings = timings
+ }Note: This requires adding a Timings field to BifrostResponseExtraFields struct.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In core/providers/openai.go around lines 239 to 242, the code incorrectly
overwrites the RawResponse field with timing data, breaking the expected
behavior when sendBackRawResponse is enabled. To fix this, add a new Timings
field to the BifrostResponseExtraFields struct and assign the timing data to
this new field instead of overwriting RawResponse. This preserves the original
RawResponse while still storing timing information separately.
532c299 to
77e050f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (12)
core/providers/openai.go (8)
48-52: Fix incorrect pool metrics tracking in New functionThe pool's
Newfunction should increment a creation counter, not the get counter. Currently, it incorrectly tracks object creation as a "get" operation.openAIResponsePool = sync.Pool{ New: func() interface{} { - openAIPoolGets.Add(1) + openAIPoolCreations.Add(1) return &schemas.BifrostResponse{} }, }
136-139: Add message preparation timingThe metrics recording expects
total_message_preparationtiming, but it's not being tracked in the ChatCompletion method.timings := make(map[string]time.Duration) + // Track message preparation time + messagePrepStart := time.Now() formattedMessages, preparedParams := prepareOpenAIChatRequest(messages, params) + timings["total_message_preparation"] = time.Since(messagePrepStart)
179-192: Critical: Production code replaced with mock responseThe actual HTTP request to OpenAI has been commented out and replaced with a mock response. This completely breaks the provider's functionality in production.
- mockResponse := mockOpenAIChatCompletionResponse(req, model) - // Copy the mock response body to the real response - resp.SetBody(mockResponse) - // Simulate network delay - jitter := time.Duration(float64(1500*time.Millisecond) * (0.6 + 0.8*rand.Float64())) - time.Sleep(jitter) - // Make request - // bifrostErr := makeRequestWithContext(ctx, provider.client, req, resp) + bifrostErr := makeRequestWithContext(ctx, provider.client, req, resp) timings["http_request"] = time.Since(httpStart) - // if bifrostErr != nil { - // provider.recordOpenAIMetrics(timings, false) // Record metrics for failed request - // return nil, bifrostErr - // } + if bifrostErr != nil { + provider.recordOpenAIMetrics(timings, false) // Record metrics for failed request + return nil, bifrostErr + }
239-242: Don't overwrite RawResponse with timing dataThe code overwrites the RawResponse field with timing data, which breaks the contract when
sendBackRawResponseis enabled. Timing data should be stored separately.Consider adding a new field to
BifrostResponseExtraFieldsstruct to store timing data separately:// In schemas package type BifrostResponseExtraFields struct { // ... existing fields ... Timings map[string]time.Duration `json:"timings,omitempty"` }Then update the code:
- response.ExtraFields.RawResponse = map[string]interface{}{ - "timings": timings, - } + response.ExtraFields.Timings = timings
1208-1225: Consider using atomic types for metrics fieldsWhile the current implementation using
atomic.AddInt64andatomic.LoadInt64is correct, consider using Go's atomic types for better type safety and cleaner code.type OpenAIMetrics struct { // Counters - RequestCount int64 - ErrorCount int64 + RequestCount atomic.Int64 + ErrorCount atomic.Int64 // Time accumulators (in nanoseconds for atomic operations) - TotalTimeNs int64 - MessagePreparationTimeNs int64 - RequestBodyPreparationTimeNs int64 - JSONMarshalingTimeNs int64 - RequestSetupTimeNs int64 - HTTPRequestTimeNs int64 - ErrorHandlingTimeNs int64 - PoolAcquisitionTimeNs int64 - JSONUnmarshalingTimeNs int64 - ResponseParsingTimeNs int64 + TotalTimeNs atomic.Int64 + MessagePreparationTimeNs atomic.Int64 + RequestBodyPreparationTimeNs atomic.Int64 + JSONMarshalingTimeNs atomic.Int64 + RequestSetupTimeNs atomic.Int64 + HTTPRequestTimeNs atomic.Int64 + ErrorHandlingTimeNs atomic.Int64 + PoolAcquisitionTimeNs atomic.Int64 + JSONUnmarshalingTimeNs atomic.Int64 + ResponseParsingTimeNs atomic.Int64 }Then update the usage to:
provider.metrics.RequestCount.Add(1) requestCount := provider.metrics.RequestCount.Load()
1348-1490: Remove mock response function from production codeThis mock response function should not be in production code. Mocking should be handled in tests or through configuration/feature flags.
Additionally:
- Uses
json.Marshalinstead ofsonic.Marshal(inconsistent with the rest of the codebase)- References undefined
StrPtrfunction- Contains excessive repetitive text
Consider:
- Moving this to a test file or test utilities package
- If mocking is needed in production, implement it via configuration/feature flags
- Use
sonic.Marshalfor consistency- Define or import the
StrPtrhelper function
1363-1363: Define the missing StrPtr helper functionThe code uses
StrPtrfunction which is not defined in this file. Add the helper function or import it from a utilities package.// Add this helper function func StrPtr(s string) *string { return &s }Also applies to: 1473-1473
11-11: Use sonic instead of json for consistencyThe codebase uses
sonicfor JSON operations throughout. Replaceencoding/jsonwithsonicfor consistency.-import ( - "encoding/json" +import (Then update line 1484 to use
sonic.Marshalinstead ofjson.Marshal.core/bifrost.go (4)
117-117: Critical: Do not disable garbage collection in production code.Calling
debug.SetGCPercent(-1)completely disables Go's garbage collector, which will cause memory to never be freed and lead to eventual out-of-memory crashes. This is dangerous for any long-running service.
1607-1641: Refactor to reduce code duplication between recordError and recordMetrics.The
recordErrormethod duplicates logic fromrecordMetrics. Consider refactoring to reuse code.
1679-1681: Consider the performance impact of runtime.ReadMemStats.
runtime.ReadMemStatsis a stop-the-world operation that pauses all goroutines. Calling it frequently (e.g., on every API response) could impact latency.
1713-1718: Consider making provider metrics collection more extensible.Currently, only OpenAI provider metrics are collected. Consider defining a
MetricsProviderinterface to make this extensible to other providers.
| account schemas.Account // account interface | ||
| plugins []schemas.Plugin // list of plugins | ||
| requestQueues map[schemas.ModelProvider]chan ChannelMessage // provider request queues (thread-safe) |
There was a problem hiding this comment.
Critical: Regular map is not thread-safe.
The change from sync.Map to a regular map[schemas.ModelProvider]chan ChannelMessage removes thread safety. This will cause race conditions when multiple goroutines access requestQueues concurrently (e.g., in getProviderQueue, prepareProvider, UpdateProviderConcurrency).
Either revert to sync.Map or add proper synchronization with a mutex:
-requestQueues map[schemas.ModelProvider]chan ChannelMessage // provider request queues (thread-safe)
+requestQueues map[schemas.ModelProvider]chan ChannelMessage // provider request queues
+requestQueuesMutex sync.RWMutex // protects requestQueuesCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In core/bifrost.go around lines 52 to 54, the change from sync.Map to a regular
map for requestQueues removes thread safety, risking race conditions during
concurrent access. To fix this, either revert requestQueues back to sync.Map or
add a mutex to synchronize all accesses and modifications to the map, ensuring
thread-safe operations in functions like getProviderQueue, prepareProvider, and
UpdateProviderConcurrency.
| if rawResponse, ok := resp.ExtraFields.RawResponse.(map[string]interface{}); ok { | ||
| rawResponse["bifrost_timings"] = bifrost.GetMetrics() | ||
| resp.ExtraFields.RawResponse = rawResponse | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add type assertion safety check.
The code assumes RawResponse is always a map[string]interface{}, but this may not be guaranteed. A failed type assertion would silently skip adding metrics.
Consider logging when the type assertion fails:
if rawResponse, ok := resp.ExtraFields.RawResponse.(map[string]interface{}); ok {
rawResponse["bifrost_timings"] = bifrost.GetMetrics()
resp.ExtraFields.RawResponse = rawResponse
+} else if resp.ExtraFields.RawResponse != nil {
+ bifrost.logger.Debug("Cannot add bifrost_timings to non-map RawResponse")
}🤖 Prompt for AI Agents
In core/bifrost.go around lines 1017 to 1020, the code assumes
resp.ExtraFields.RawResponse is always a map[string]interface{} without
verifying it, which can cause silent failures if the assertion fails. Add an
else branch to the type assertion that logs a warning or error indicating the
type assertion failed, so that such cases are detected and can be investigated.
This will improve observability and help catch unexpected RawResponse types.
0883fb9 to
747f1b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (12)
core/providers/openai.go (7)
49-49: Fix incorrect pool metrics tracking in New functionThe pool's
Newfunction should increment a creation counter, not the get counter. Currently, it incorrectly tracks object creation as a "get" operation.- openAIPoolGets.Add(1) + openAIPoolCreations.Add(1)
138-138: Add message preparation timingThe metrics recording expects
total_message_preparationtiming, but it's not being tracked in the ChatCompletion method.+ // Track message preparation time + messagePrepStart := time.Now() formattedMessages, preparedParams := prepareOpenAIChatRequest(messages, params) + timings["total_message_preparation"] = time.Since(messagePrepStart)
179-192: Critical: Production code replaced with mock responseThe actual HTTP request to OpenAI has been commented out and replaced with a mock response. This completely breaks the provider's functionality in production.
- mockResponse := mockOpenAIChatCompletionResponse(req, model) - // Copy the mock response body to the real response - resp.SetBody(mockResponse) - // Simulate network delay - jitter := time.Duration(float64(1500*time.Millisecond) * (0.6 + 0.8*rand.Float64())) - time.Sleep(jitter) - // Make request - // bifrostErr := makeRequestWithContext(ctx, provider.client, req, resp) + bifrostErr := makeRequestWithContext(ctx, provider.client, req, resp) timings["http_request"] = time.Since(httpStart) - // if bifrostErr != nil { - // provider.recordOpenAIMetrics(timings, false) // Record metrics for failed request - // return nil, bifrostErr - // } + if bifrostErr != nil { + provider.recordOpenAIMetrics(timings, false) // Record metrics for failed request + return nil, bifrostErr + }
239-241: Don't overwrite RawResponse with timing dataThe code overwrites the RawResponse field with timing data, which breaks the contract when
sendBackRawResponseis enabled. Timing data should be stored separately.This requires adding a new field to the
BifrostResponseExtraFieldsstruct to store timing data separately from the raw response.
1208-1225: Consider using atomic types for metrics fieldsWhile the current implementation using
atomic.AddInt64andatomic.LoadInt64is correct, consider using Go's atomic types for better type safety and cleaner code.type OpenAIMetrics struct { // Counters - RequestCount int64 - ErrorCount int64 + RequestCount atomic.Int64 + ErrorCount atomic.Int64 // Time accumulators (in nanoseconds for atomic operations) - TotalTimeNs int64 - MessagePreparationTimeNs int64 - RequestBodyPreparationTimeNs int64 - JSONMarshalingTimeNs int64 - RequestSetupTimeNs int64 - HTTPRequestTimeNs int64 - ErrorHandlingTimeNs int64 - PoolAcquisitionTimeNs int64 - JSONUnmarshalingTimeNs int64 - ResponseParsingTimeNs int64 + TotalTimeNs atomic.Int64 + MessagePreparationTimeNs atomic.Int64 + RequestBodyPreparationTimeNs atomic.Int64 + JSONMarshalingTimeNs atomic.Int64 + RequestSetupTimeNs atomic.Int64 + HTTPRequestTimeNs atomic.Int64 + ErrorHandlingTimeNs atomic.Int64 + PoolAcquisitionTimeNs atomic.Int64 + JSONUnmarshalingTimeNs atomic.Int64 + ResponseParsingTimeNs atomic.Int64 }Then update the usage to:
provider.metrics.RequestCount.Add(1) requestCount := provider.metrics.RequestCount.Load()
1348-1490: Remove mock response function from production codeThis mock response function should not be in production code. Mocking should be handled in tests or through configuration/feature flags.
Additionally:
- Uses
json.Marshalinstead ofsonic.Marshal(inconsistent with the rest of the codebase)- References undefined
StrPtrfunction- Contains excessive repetitive text
Consider:
- Moving this to a test file or test utilities package
- If mocking is needed in production, implement it via configuration/feature flags
- Use
sonic.Marshalfor consistency- Define or import the
StrPtrhelper function
11-11: Use sonic.Marshal instead of json.Marshal for consistencyThe codebase consistently uses
sonicfor JSON operations. Replacejson.Marshalwithsonic.Marshalin the mock response function to maintain consistency.- "encoding/json"And in the mock function:
- mockJSON, err := json.Marshal(mockResp) + mockJSON, err := sonic.Marshal(mockResp)Also applies to: 1484-1484
core/bifrost.go (5)
1597-1631: Refactor to reduce code duplication between recordError and recordMetrics.The
recordErrormethod duplicates logic fromrecordMetrics. Consider refactoring to reuse code.Apply this refactor:
-func (bifrost *Bifrost) recordError(queueWaitTime, keySelectTime, providerTime, pluginPreTime, pluginPostTime time.Duration) { - // Spawn goroutine to avoid blocking the request path - go func() { - atomic.AddInt64(&bifrost.metrics.RequestCount, 1) - atomic.AddInt64(&bifrost.metrics.ErrorCount, 1) - - totalTime := queueWaitTime + keySelectTime + providerTime + pluginPreTime + pluginPostTime - - // Add to accumulators - atomic.AddInt64(&bifrost.metrics.TotalTimeNs, totalTime.Nanoseconds()) - atomic.AddInt64(&bifrost.metrics.QueueWaitTimeNs, queueWaitTime.Nanoseconds()) - atomic.AddInt64(&bifrost.metrics.KeySelectionTimeNs, keySelectTime.Nanoseconds()) - atomic.AddInt64(&bifrost.metrics.ProviderTimeNs, providerTime.Nanoseconds()) - atomic.AddInt64(&bifrost.metrics.PluginPreTimeNs, pluginPreTime.Nanoseconds()) - atomic.AddInt64(&bifrost.metrics.PluginPostTimeNs, pluginPostTime.Nanoseconds()) - }() +func (bifrost *Bifrost) recordError(queueWaitTime, keySelectTime, providerTime, pluginPreTime, pluginPostTime time.Duration) { + totalTime := queueWaitTime + keySelectTime + providerTime + pluginPreTime + pluginPostTime + bifrost.recordMetrics(queueWaitTime, keySelectTime, providerTime, pluginPreTime, pluginPostTime, totalTime, false) }
1669-1671: Consider the performance impact of runtime.ReadMemStats.
runtime.ReadMemStatsis a stop-the-world operation that pauses all goroutines. Calling it frequently (e.g., on every API response) could impact latency.Consider these alternatives:
- Cache the memory stats and update them periodically in a background goroutine
- Make memory stats collection optional via a parameter
- Use lighter-weight alternatives like
runtime.MemStats.AllocdirectlyExample implementation with caching:
// Add to Bifrost struct memStatsCache struct { sync.RWMutex stats runtime.MemStats lastUpdate time.Time } // Update periodically in background func (bifrost *Bifrost) updateMemStatsCache() { ticker := time.NewTicker(5 * time.Second) go func() { for range ticker.C { var stats runtime.MemStats runtime.ReadMemStats(&stats) bifrost.memStatsCache.Lock() bifrost.memStatsCache.stats = stats bifrost.memStatsCache.lastUpdate = time.Now() bifrost.memStatsCache.Unlock() } }() }
1703-1708: Consider making provider metrics collection more extensible.Currently, only OpenAI provider metrics are collected. Consider defining a
MetricsProviderinterface to make this extensible to other providers.Example approach:
// Define interface type MetricsProvider interface { GetProviderMetrics() map[string]interface{} } // In GetAllStats bifrost.providerInstances.Range(func(key, value interface{}) bool { if metricsProvider, ok := value.(MetricsProvider); ok { providerKey := key.(schemas.ModelProvider) stats["provider_metrics"].(map[string]interface{})[string(providerKey)] = metricsProvider.GetProviderMetrics() } return true })
1009-1012: Add type assertion safety check.The code assumes
RawResponseis always amap[string]interface{}, but this may not be guaranteed. A failed type assertion would silently skip adding metrics.Consider logging when the type assertion fails:
if rawResponse, ok := resp.ExtraFields.RawResponse.(map[string]interface{}); ok { rawResponse["bifrost_timings"] = bifrost.GetMetrics() resp.ExtraFields.RawResponse = rawResponse +} else if resp.ExtraFields.RawResponse != nil { + bifrost.logger.Debug("Cannot add bifrost_timings to non-map RawResponse") }
117-117: Critical: Do not disable garbage collection in production code.Calling
debug.SetGCPercent(-1)completely disables Go's garbage collector, which will cause memory to never be freed and lead to eventual out-of-memory crashes. This is dangerous for any long-running service.If you need to tune GC for performance, consider using a higher percentage (e.g.,
debug.SetGCPercent(200)) instead of disabling it entirely. Alternatively, use theGOGCenvironment variable to allow runtime configuration.
| dropExcessRequests atomic.Bool // If true, in cases where the queue is full, requests will not wait for the queue to be empty and will be dropped instead. | ||
| account schemas.Account // account interface | ||
| plugins []schemas.Plugin // list of plugins | ||
| openAIChannel chan ChannelMessage // provider request queues (thread-safe) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Update the comment to reflect the new single-channel architecture.
The comment mentions "provider request queues (thread-safe)" but the field is now a single channel for all providers, not multiple queues. Channels are inherently thread-safe, but the comment should be updated to reflect the architectural change.
- openAIChannel chan ChannelMessage // provider request queues (thread-safe)
+ openAIChannel chan ChannelMessage // single request queue for all providers📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| openAIChannel chan ChannelMessage // provider request queues (thread-safe) | |
| openAIChannel chan ChannelMessage // single request queue for all providers |
🤖 Prompt for AI Agents
In core/bifrost.go at line 54, update the comment for the openAIChannel field to
reflect that it is now a single channel used for all providers, rather than
multiple provider request queues. Remove the mention of "queues" and clarify
that this single channel handles provider requests in a thread-safe manner, as
channels are inherently thread-safe.
|
|
||
| // Step 4: Atomically replace the queue | ||
| bifrost.requestQueues.Store(providerKey, newQueue) | ||
| bifrost.openAIChannel = newQueue |
There was a problem hiding this comment.
Architecture change: All providers now share a single queue.
The change from per-provider queues to a single openAIChannel fundamentally changes the queuing behavior. This means:
- All providers share the same buffer and concurrency limits
UpdateProviderConcurrencyaffects all providers, not just the specified one- High traffic on one provider can starve others
Is this intentional? If so, consider renaming openAIChannel to something more generic like requestChannel.
Also applies to: 733-733, 761-761
🤖 Prompt for AI Agents
In core/bifrost.go at lines 433, 733, and 761, the variable name `openAIChannel`
is misleading because it now represents a shared queue for all providers rather
than just OpenAI. Rename `openAIChannel` to a more generic name like
`requestChannel` to accurately reflect that it is a single shared queue for all
providers, ensuring the naming matches the new architecture and avoids
confusion.
747f1b4 to
5a00ca3
Compare
Pratham-Mishra04
changed the base branch from
graphite-base/211
to
08-05-enhancement_core_implementation_optimsations
Pratham-Mishra04
force-pushed
the
08-05-enhancement_core_implementation_optimsations
branch
from
b80bf17 to
aef63f8
Compare
Pratham-Mishra04
force-pushed
the
07-29-dev_performance_tracking_for_benchmarking
branch
2 times, most recently
from
885e7f0 to
c001075
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (11)
core/providers/openai.go (6)
48-52: Fix incorrect pool metrics tracking in New functionThe pool's
Newfunction should increment a creation counter, not the get counter. Currently, it incorrectly tracks object creation as a "get" operation.
136-139: Add message preparation timingThe metrics recording expects
total_message_preparationtiming, but it's not being tracked in the ChatCompletion method.
179-193: Critical: Production code replaced with mock responseThe actual HTTP request to OpenAI has been commented out and replaced with a mock response. This completely breaks the provider's functionality in production.
239-242: Don't overwrite RawResponse with timing dataThe code overwrites the RawResponse field with timing data, which breaks the contract when
sendBackRawResponseis enabled. Timing data should be stored separately.
1208-1225: Consider using atomic types for metrics fieldsWhile the current implementation using
atomic.AddInt64andatomic.LoadInt64is correct, consider using Go's atomic types for better type safety and cleaner code.
1348-1490: Remove mock response function from production codeThis mock response function should not be in production code. Mocking should be handled in tests or through configuration/feature flags.
Additionally:
- Uses
json.Marshalinstead ofsonic.Marshal(inconsistent with the rest of the codebase)- References undefined
StrPtrfunction- Contains excessive repetitive text
core/bifrost.go (5)
130-130: Critical: Do not disable garbage collection in production code.Calling
debug.SetGCPercent(-1)completely disables Go's garbage collector, which will cause memory to never be freed and lead to eventual out-of-memory crashes.If you need to tune GC for performance, consider using a higher percentage (e.g.,
debug.SetGCPercent(200)) instead of disabling it entirely.
1105-1108: Add type assertion safety check.The code assumes
RawResponseis always amap[string]interface{}. Consider logging when the type assertion fails:if rawResponse, ok := resp.ExtraFields.RawResponse.(map[string]interface{}); ok { rawResponse["bifrost_timings"] = bifrost.GetMetrics() resp.ExtraFields.RawResponse = rawResponse +} else if resp.ExtraFields.RawResponse != nil { + bifrost.logger.Debug("Cannot add bifrost_timings to non-map RawResponse") }
1708-1724: Refactor to reduce code duplication between recordError and recordMetrics.The
recordErrormethod duplicates logic fromrecordMetrics. Consider refactoring to reuse code:-func (bifrost *Bifrost) recordError(queueWaitTime, keySelectTime, providerTime, pluginPreTime, pluginPostTime time.Duration) { - // Spawn goroutine to avoid blocking the request path - go func() { - atomic.AddInt64(&bifrost.metrics.RequestCount, 1) - atomic.AddInt64(&bifrost.metrics.ErrorCount, 1) - - totalTime := queueWaitTime + keySelectTime + providerTime + pluginPreTime + pluginPostTime - - // Add to accumulators - atomic.AddInt64(&bifrost.metrics.TotalTimeNs, totalTime.Nanoseconds()) - atomic.AddInt64(&bifrost.metrics.QueueWaitTimeNs, queueWaitTime.Nanoseconds()) - atomic.AddInt64(&bifrost.metrics.KeySelectionTimeNs, keySelectTime.Nanoseconds()) - atomic.AddInt64(&bifrost.metrics.ProviderTimeNs, providerTime.Nanoseconds()) - atomic.AddInt64(&bifrost.metrics.PluginPreTimeNs, pluginPreTime.Nanoseconds()) - atomic.AddInt64(&bifrost.metrics.PluginPostTimeNs, pluginPostTime.Nanoseconds()) - }() +func (bifrost *Bifrost) recordError(queueWaitTime, keySelectTime, providerTime, pluginPreTime, pluginPostTime time.Duration) { + totalTime := queueWaitTime + keySelectTime + providerTime + pluginPreTime + pluginPostTime + bifrost.recordMetrics(queueWaitTime, keySelectTime, providerTime, pluginPreTime, pluginPostTime, totalTime, false) }
1782-1784: Consider the performance impact of runtime.ReadMemStats.
runtime.ReadMemStatsis a stop-the-world operation that pauses all goroutines. Consider caching the memory stats and updating them periodically in a background goroutine.
1816-1821: Consider making provider metrics collection more extensible.Currently, only OpenAI provider metrics are collected. Consider defining a
MetricsProviderinterface to make this extensible to other providers.
| atomic.AddInt64(&bifrost.metrics.ErrorCount, 1) | ||
| } | ||
|
|
||
| // fmt.Println(queueWaitTime) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove commented debug statement.
- // fmt.Println(queueWaitTime)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // fmt.Println(queueWaitTime) |
🤖 Prompt for AI Agents
In core/bifrost.go at line 1734, remove the commented out debug statement
"// fmt.Println(queueWaitTime)" to clean up the code and avoid leaving
unnecessary commented code.
| SpeechInput *schemas.SpeechInput | ||
| TranscriptionInput *schemas.TranscriptionInput | ||
| Tools *[]schemas.Tool | ||
| Tools []schemas.Tool |
There was a problem hiding this comment.
Breaking change: Tools field type modified.
The Tools field type changed from *[]schemas.Tool (pointer to slice) to []schemas.Tool (direct slice). This is a breaking change that affects any code expecting the previous pointer semantics.
Ensure this type change is intentional and update any dependent code accordingly.
🤖 Prompt for AI Agents
In transports/bifrost-http/plugins/logging/main.go at line 83, the Tools field
type was changed from a pointer to a slice (*[]schemas.Tool) to a direct slice
([]schemas.Tool). Confirm this change is intentional and then update all code
that accesses or modifies Tools to handle it as a direct slice instead of a
pointer, removing any dereferencing or pointer usage related to Tools.
c001075 to
a5457e1
Compare
Pratham-Mishra04
force-pushed
the
07-29-dev_performance_tracking_for_benchmarking
branch
from
a5457e1 to
6f8635d
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (13)
transports/bifrost-http/plugins/logging/main.go (1)
83-83: Breaking change: Tools field type modified.The
Toolsfield type changed from*[]schemas.Tool(pointer to slice) to[]schemas.Tool(direct slice). This is a breaking change that affects any code expecting the previous pointer semantics.Ensure this type change is intentional and update any dependent code accordingly.
core/providers/openai.go (6)
48-52: Fix incorrect pool metrics tracking in New functionThe pool's
Newfunction should increment a creation counter, not the get counter. Currently, it incorrectly tracks object creation as a "get" operation.openAIResponsePool = sync.Pool{ New: func() interface{} { - openAIPoolGets.Add(1) + openAIPoolCreations.Add(1) return &schemas.BifrostResponse{} }, }
136-139: Add message preparation timingThe metrics recording expects
total_message_preparationtiming, but it's not being tracked in the ChatCompletion method.timings := make(map[string]time.Duration) + // Track message preparation time + messagePrepStart := time.Now() formattedMessages, preparedParams := prepareOpenAIChatRequest(messages, params) + timings["total_message_preparation"] = time.Since(messagePrepStart)
179-192: Critical: Production code replaced with mock responseThe actual HTTP request to OpenAI has been commented out and replaced with a mock response. This completely breaks the provider's functionality in production.
- mockResponse := mockOpenAIChatCompletionResponse(req, model) - // Copy the mock response body to the real response - resp.SetBody(mockResponse) - // Simulate network delay - jitter := time.Duration(float64(1500*time.Millisecond) * (0.6 + 0.8*rand.Float64())) - time.Sleep(jitter) - // Make request - // bifrostErr := makeRequestWithContext(ctx, provider.client, req, resp) + bifrostErr := makeRequestWithContext(ctx, provider.client, req, resp) timings["http_request"] = time.Since(httpStart) - // if bifrostErr != nil { - // provider.recordOpenAIMetrics(timings, false) // Record metrics for failed request - // return nil, bifrostErr - // } + if bifrostErr != nil { + provider.recordOpenAIMetrics(timings, false) // Record metrics for failed request + return nil, bifrostErr + }
239-242: Don't overwrite RawResponse with timing dataThe code overwrites the RawResponse field with timing data, which breaks the contract when
sendBackRawResponseis enabled. Timing data should be stored separately.Add a
Timingsfield toBifrostResponseExtraFieldsstruct and use:- response.ExtraFields.RawResponse = map[string]interface{}{ - "timings": timings, - } + response.ExtraFields.Timings = timings
1208-1225: Consider using atomic types for metrics fieldsWhile the current implementation using
atomic.AddInt64andatomic.LoadInt64is correct, consider using Go's atomic types for better type safety and cleaner code.type OpenAIMetrics struct { // Counters - RequestCount int64 - ErrorCount int64 + RequestCount atomic.Int64 + ErrorCount atomic.Int64 // Time accumulators (in nanoseconds for atomic operations) - TotalTimeNs int64 - MessagePreparationTimeNs int64 - RequestBodyPreparationTimeNs int64 - JSONMarshalingTimeNs int64 - RequestSetupTimeNs int64 - HTTPRequestTimeNs int64 - ErrorHandlingTimeNs int64 - PoolAcquisitionTimeNs int64 - JSONUnmarshalingTimeNs int64 - ResponseParsingTimeNs int64 + TotalTimeNs atomic.Int64 + MessagePreparationTimeNs atomic.Int64 + RequestBodyPreparationTimeNs atomic.Int64 + JSONMarshalingTimeNs atomic.Int64 + RequestSetupTimeNs atomic.Int64 + HTTPRequestTimeNs atomic.Int64 + ErrorHandlingTimeNs atomic.Int64 + PoolAcquisitionTimeNs atomic.Int64 + JSONUnmarshalingTimeNs atomic.Int64 + ResponseParsingTimeNs atomic.Int64 }Then update usage to:
provider.metrics.RequestCount.Add(1) requestCount := provider.metrics.RequestCount.Load()
1348-1490: Remove mock response function from production codeThis mock response function should not be in production code. Mocking should be handled in tests or through configuration/feature flags.
Additionally:
- Uses
json.Marshalinstead ofsonic.Marshal(inconsistent with the rest of the codebase)- References undefined
StrPtrfunction- Contains excessive repetitive text
Consider:
- Moving this to a test file or test utilities package
- If mocking is needed in production, implement it via configuration/feature flags
- Use
sonic.Marshalfor consistency- Define or import the
StrPtrhelper functioncore/bifrost.go (6)
130-130: Critical: Do not disable garbage collection in production code.This issue was already raised in previous reviews but hasn't been addressed. Disabling GC will cause memory leaks and crashes.
1105-1108: Add type assertion safety check.This issue was already raised in previous reviews. The code assumes RawResponse is always a map[string]interface{}.
1710-1727: Refactor to reduce code duplication between recordError and recordMetrics.This duplication was already identified in previous reviews. The recordError method should call recordMetrics.
1737-1737: Remove commented debug statement.- // fmt.Println("Queue acquisition:", queueAcquisitionTime, "Queue wait:", queueWaitTime)
1788-1790: Consider the performance impact of runtime.ReadMemStats.This stop-the-world operation was already flagged in previous reviews. Consider caching or making it optional.
1822-1827: Consider making provider metrics collection more extensible.Currently hardcoded for OpenAI only. This limitation was already identified in previous reviews.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
core/bifrost.go(22 hunks)core/providers/openai.go(10 hunks)transports/bifrost-http/main.go(2 hunks)transports/bifrost-http/plugins/logging/main.go(3 hunks)transports/bifrost-http/plugins/logging/operations.go(1 hunks)transports/go.mod(1 hunks)
🔇 Additional comments (10)
transports/bifrost-http/main.go (1)
431-453: Well-implemented graceful shutdown!The graceful shutdown implementation properly handles OS signals, cleans up resources (WebSocket handler and Bifrost client), and provides clear logging. This is essential for production deployments.
core/bifrost.go (9)
46-46: Good addition for queue wait time tracking.The Timestamp field enables accurate measurement of how long messages wait in the queue before processing.
78-99: Well-designed performance tracking implementation.The atomic counters and sync.Map usage provide thread-safe, low-overhead metrics collection suitable for high-throughput scenarios.
146-178: Correct implementation of pool creation tracking.The atomic increments in pool New functions accurately track object creation rates, helping identify pool efficiency.
467-468: Proper storage of provider instances for metrics access.Using sync.Map ensures thread-safe access to provider instances when collecting metrics.
Also applies to: 758-759
1216-1325: Comprehensive request timing implementation.The timing captures all stages of request processing including queue wait, key selection, provider time, and retry overhead. Recording metrics for both success and failure cases ensures accurate statistics.
1482-1566: Correct implementation of pool usage tracking.All pool get/put operations are properly instrumented with atomic counters. The timestamp is set at the right point for queue wait time measurement.
1668-1674: Good addition of final statistics logging.Logging comprehensive statistics in JSON format during cleanup provides valuable insights into system performance.
1750-1768: Well-implemented metrics calculation.The method safely calculates averages using atomic operations and handles the zero requests case properly.
1832-1871: Comprehensive pool statistics collection.The method properly collects statistics from all pools and includes provider-specific stats, providing a complete view of pool usage.
| // ADD THIS DEBUG LINE | ||
| bifrost.logger.Warn(fmt.Sprintf("UNEXPECTED: Provider %s queue is nil after Init()!", providerKey)) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Review the log level and necessity of this debug statement.
The comment says "ADD THIS DEBUG LINE" but uses Warn level. Either:
- Remove if this was temporary debugging
- Use Debug level if keeping for diagnostics
- Keep as Warn if this represents an actual unexpected condition
🤖 Prompt for AI Agents
In core/bifrost.go around lines 838 to 839, the log statement uses Warn level
but is labeled as a debug line. Decide if this message indicates a real
unexpected condition; if so, keep Warn level. If it is only for diagnostic
purposes, change the log level to Debug. If it was temporary debugging, remove
the log statement entirely.
| // if req.Params != nil && req.Params.Tools != nil { | ||
| // initialData.Tools = req.Params.Tools | ||
| // } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented code for Tools assignment.
The Tools assignment is commented out but the field still exists in the struct. Either remove the field entirely or implement the assignment properly.
Apply this diff to clean up:
- // if req.Params != nil && req.Params.Tools != nil {
- // initialData.Tools = req.Params.Tools
- // }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // if req.Params != nil && req.Params.Tools != nil { | |
| // initialData.Tools = req.Params.Tools | |
| // } |
🤖 Prompt for AI Agents
In transports/bifrost-http/plugins/logging/main.go around lines 334 to 336,
there is commented-out code assigning Tools from req.Params to
initialData.Tools. Since this code is commented out but the Tools field still
exists in the struct, either remove the Tools field from the struct entirely if
it is unused, or uncomment and properly implement the assignment to ensure the
field is correctly populated. Choose one approach to clean up the code and
maintain consistency.
| // ToolsParsed: logMsg.InitialData.Tools, | ||
| Status: "processing", | ||
| Stream: false, // Initially false, will be updated if streaming | ||
| CreatedAt: logMsg.Timestamp, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented ToolsParsed assignment.
Clean up the commented code to maintain code clarity.
Apply this diff:
InputHistoryParsed: logMsg.InitialData.InputHistory,
ParamsParsed: logMsg.InitialData.Params,
- // ToolsParsed: logMsg.InitialData.Tools,
Status: "processing",
Stream: false, // Initially false, will be updated if streaming
CreatedAt: logMsg.Timestamp,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // ToolsParsed: logMsg.InitialData.Tools, | |
| Status: "processing", | |
| Stream: false, // Initially false, will be updated if streaming | |
| CreatedAt: logMsg.Timestamp, | |
| InputHistoryParsed: logMsg.InitialData.InputHistory, | |
| ParamsParsed: logMsg.InitialData.Params, | |
| Status: "processing", | |
| Stream: false, // Initially false, will be updated if streaming | |
| CreatedAt: logMsg.Timestamp, |
🤖 Prompt for AI Agents
In transports/bifrost-http/plugins/logging/main.go around lines 366 to 369,
remove the commented out line assigning ToolsParsed from
logMsg.InitialData.Tools to clean up the code and improve clarity.
| InputHistoryParsed: data.InputHistory, | ||
| ParamsParsed: data.Params, | ||
| // ToolsParsed: data.Tools, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove or document commented code.
The ToolsParsed assignment is commented out. If this is intentional, remove the line entirely. If it's temporarily disabled, add a TODO comment explaining why.
Apply this diff to clean up the code:
InputHistoryParsed: data.InputHistory,
ParamsParsed: data.Params,
- // ToolsParsed: data.Tools,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| InputHistoryParsed: data.InputHistory, | |
| ParamsParsed: data.Params, | |
| // ToolsParsed: data.Tools, | |
| InputHistoryParsed: data.InputHistory, | |
| ParamsParsed: data.Params, |
🤖 Prompt for AI Agents
In transports/bifrost-http/plugins/logging/operations.go around lines 28 to 30,
the ToolsParsed assignment line is commented out. Either remove this commented
line completely if it is no longer needed, or if it is temporarily disabled, add
a clear TODO comment explaining why it is commented out and when it should be
revisited.
| gorm.io/gorm v1.30.0 | ||
| ) | ||
|
|
||
| replace github.com/maximhq/bifrost/core => ../core |
There was a problem hiding this comment.
Remove replace directive before merging to production.
The replace directive should only be used for local development and testing. It will break builds for other developers and CI/CD systems that don't have the same directory structure.
Apply this diff to remove the local replace directive:
-replace github.com/maximhq/bifrost/core => ../core📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| replace github.com/maximhq/bifrost/core => ../core |
🤖 Prompt for AI Agents
In transports/go.mod at line 19, remove the replace directive "replace
github.com/maximhq/bifrost/core => ../core" because it is intended only for
local development and will cause build failures in other environments. Delete
this line entirely before merging to production to ensure builds work correctly
for all developers and CI/CD systems.
|
|
2 participants
Performance Metrics and Monitoring System
This PR adds comprehensive performance tracking and metrics collection to Bifrost, enabling detailed insights into system performance and resource usage. Key improvements include:
These changes will help identify performance bottlenecks, optimize resource usage, and provide valuable insights for capacity planning. The metrics are collected with minimal overhead using atomic operations and are accessible through the API responses and logs.