-
Notifications
You must be signed in to change notification settings - Fork 3.2k
perf: add inventory cache for stateless server patterns #1636
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
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 |
|---|---|---|
| @@ -0,0 +1,165 @@ | ||
| package github | ||
|
|
||
| import ( | ||
| "sync" | ||
|
|
||
| "github.com/github/github-mcp-server/pkg/inventory" | ||
| "github.com/github/github-mcp-server/pkg/translations" | ||
| ) | ||
|
|
||
| // CachedInventory provides a cached inventory builder that builds tool definitions | ||
| // only once, regardless of how many times CachedInventoryBuilder is called. | ||
| // | ||
| // This is particularly useful for stateless server patterns (like the remote server) | ||
| // where a new server instance is created per request. Without caching, every request | ||
| // would rebuild all ~130 tool definitions including JSON schema generation, causing | ||
| // significant performance overhead. | ||
| // | ||
| // Usage: | ||
| // | ||
| // // Option 1: Initialize once at startup with your translator | ||
| // github.InitInventoryCache(myTranslator) | ||
| // | ||
| // // Then get pre-built inventory on each request | ||
| // inv := github.CachedInventoryBuilder(). | ||
| // WithReadOnly(cfg.ReadOnly). | ||
| // WithToolsets(cfg.Toolsets). | ||
| // Build() | ||
| // | ||
| // // Option 2: Use NewInventory which doesn't use the cache (legacy behavior) | ||
| // inv := github.NewInventory(myTranslator).Build() | ||
| // | ||
| // The cache stores the built []ServerTool, []ServerResourceTemplate, and []ServerPrompt. | ||
| // Per-request configuration (read-only, toolsets, feature flags, filters) is still | ||
| // applied when building the Inventory from the cached data. | ||
| type CachedInventory struct { | ||
| once sync.Once | ||
| initialized bool // set to true after init completes | ||
| tools []inventory.ServerTool | ||
| resources []inventory.ServerResourceTemplate | ||
| prompts []inventory.ServerPrompt | ||
| } | ||
|
|
||
| // global singleton for caching | ||
| var globalInventoryCache = &CachedInventory{} | ||
|
|
||
| // InitInventoryCache initializes the global inventory cache with the given translator. | ||
| // This should be called once at startup before any requests are processed. | ||
| // It's safe to call multiple times - only the first call has any effect. | ||
| // | ||
| // For the local server, this is typically called with the configured translator. | ||
| // For the remote server, use translations.NullTranslationHelper since translations | ||
| // aren't needed per-request. | ||
| // | ||
| // Example: | ||
| // | ||
| // func main() { | ||
| // t, _ := translations.TranslationHelper() | ||
| // github.InitInventoryCache(t) | ||
| // // ... start server | ||
| // } | ||
| func InitInventoryCache(t translations.TranslationHelperFunc) { | ||
| globalInventoryCache.init(t, nil, nil, nil) | ||
| } | ||
|
|
||
| // InitInventoryCacheWithExtras initializes the global inventory cache with the given | ||
| // translator plus additional tools, resources, and prompts. | ||
| // | ||
| // This is useful for the remote server which has additional tools (e.g., Copilot tools) | ||
| // that aren't part of the base github-mcp-server package. | ||
| // | ||
| // The extra items are appended to the base items from AllTools/AllResources/AllPrompts. | ||
| // It's safe to call multiple times - only the first call has any effect. | ||
| // | ||
| // Example: | ||
| // | ||
| // func init() { | ||
| // github.InitInventoryCacheWithExtras( | ||
| // translations.NullTranslationHelper, | ||
| // remoteOnlyTools, // []inventory.ServerTool | ||
| // remoteOnlyResources, // []inventory.ServerResourceTemplate | ||
| // remoteOnlyPrompts, // []inventory.ServerPrompt | ||
| // ) | ||
| // } | ||
| func InitInventoryCacheWithExtras( | ||
| t translations.TranslationHelperFunc, | ||
| extraTools []inventory.ServerTool, | ||
| extraResources []inventory.ServerResourceTemplate, | ||
| extraPrompts []inventory.ServerPrompt, | ||
| ) { | ||
| globalInventoryCache.init(t, extraTools, extraResources, extraPrompts) | ||
| } | ||
|
|
||
| // init initializes the cache with the given translator and optional extras (sync.Once protected). | ||
| func (c *CachedInventory) init( | ||
| t translations.TranslationHelperFunc, | ||
| extraTools []inventory.ServerTool, | ||
| extraResources []inventory.ServerResourceTemplate, | ||
| extraPrompts []inventory.ServerPrompt, | ||
| ) { | ||
| c.once.Do(func() { | ||
| c.tools = AllTools(t) | ||
| c.resources = AllResources(t) | ||
| c.prompts = AllPrompts(t) | ||
|
|
||
| // Append extra items if provided | ||
| if len(extraTools) > 0 { | ||
| c.tools = append(c.tools, extraTools...) | ||
| } | ||
| if len(extraResources) > 0 { | ||
|
Comment on lines
+106
to
+109
|
||
| c.resources = append(c.resources, extraResources...) | ||
| } | ||
| if len(extraPrompts) > 0 { | ||
| c.prompts = append(c.prompts, extraPrompts...) | ||
| } | ||
|
|
||
| c.initialized = true | ||
| }) | ||
| } | ||
|
|
||
|
Comment on lines
+117
to
+119
|
||
| // CachedInventoryBuilder returns an inventory.Builder pre-populated with cached | ||
| // tool/resource/prompt definitions. | ||
| // | ||
| // The cache should typically be initialized via InitInventoryCache at startup. | ||
| // If not already initialized when this function is called, it will automatically | ||
| // initialize with NullTranslationHelper as a fallback. | ||
| // | ||
| // Per-request configuration can still be applied via the builder methods: | ||
| // - WithReadOnly(bool) - filter to read-only tools | ||
| // - WithToolsets([]string) - enable specific toolsets | ||
| // - WithTools([]string) - enable specific tools | ||
| // - WithFeatureChecker(func) - per-request feature flag evaluation | ||
| // - WithFilter(func) - custom filtering | ||
| // | ||
| // Example: | ||
| // | ||
| // inv := github.CachedInventoryBuilder(). | ||
| // WithReadOnly(cfg.ReadOnly). | ||
| // WithToolsets(cfg.EnabledToolsets). | ||
| // WithFeatureChecker(createFeatureChecker(cfg.EnabledFeatures)). | ||
| // Build() | ||
| func CachedInventoryBuilder() *inventory.Builder { | ||
| // Ensure cache is initialized (with NullTranslationHelper as fallback) | ||
| globalInventoryCache.init(translations.NullTranslationHelper, nil, nil, nil) | ||
|
|
||
| return inventory.NewBuilder(). | ||
| SetTools(globalInventoryCache.tools). | ||
| SetResources(globalInventoryCache.resources). | ||
| SetPrompts(globalInventoryCache.prompts) | ||
| } | ||
|
|
||
| // IsCacheInitialized returns true if the inventory cache has been initialized. | ||
| // This is primarily useful for testing. | ||
| func IsCacheInitialized() bool { | ||
| return globalInventoryCache.initialized | ||
| } | ||
|
|
||
| // ResetInventoryCache resets the global inventory cache, allowing it to be | ||
| // reinitialized with a different translator. | ||
| // | ||
| // WARNING: This function is for testing only. It is NOT thread-safe and must only | ||
| // be called when no other goroutines are accessing the cache. Tests using this | ||
| // function must not use t.Parallel(). | ||
| func ResetInventoryCache() { | ||
| globalInventoryCache = &CachedInventory{} | ||
| } | ||
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.
The documentation is contradictory. Line 76 states "The cache must be initialized via InitInventoryCache before calling this function" but line 77 immediately says "If the cache is not initialized, this will initialize it with NullTranslationHelper."
This creates confusion about whether initialization is required or optional. Consider rephrasing to clarify the behavior, such as: "The cache should typically be initialized via InitInventoryCache before calling this function. If not already initialized, it will automatically initialize with NullTranslationHelper as a fallback."