diff --git a/packages/core/src/core/loggingContentGenerator/loggingContentGenerator.test.ts b/packages/core/src/core/loggingContentGenerator/loggingContentGenerator.test.ts index 8c6413b7ec9..859c0094e82 100644 --- a/packages/core/src/core/loggingContentGenerator/loggingContentGenerator.test.ts +++ b/packages/core/src/core/loggingContentGenerator/loggingContentGenerator.test.ts @@ -1440,8 +1440,13 @@ describe('LoggingContentGenerator', () => { expect(openaiLoggerInstance.logInteraction).toHaveBeenCalledTimes(1); }); - it.each(['prompt_suggestion', 'forked_query', 'speculation'])( - 'skips logApiRequest and OpenAI logging for internal promptId %s (generateContent)', + it.each([ + 'prompt_suggestion', + 'forked_query', + 'speculation', + 'side-query:session-title', + ])( + 'skips logApiRequest but writes tagged OpenAI logging for internal promptId %s (generateContent)', async (promptId) => { const mockResponse = { responseId: 'internal-resp', @@ -1474,17 +1479,36 @@ describe('LoggingContentGenerator', () => { expect(logApiResponse).toHaveBeenCalled(); const [, responseEvent] = vi.mocked(logApiResponse).mock.calls[0]; expect(responseEvent.response_text).toBeUndefined(); - // OpenAI logger should be constructed, but no interaction should be logged + // OpenAI file logging is explicit diagnostic output, so internal prompts + // are written with a tag instead of being dropped. expect(OpenAILogger).toHaveBeenCalled(); const loggerInstance = ( OpenAILogger as unknown as ReturnType ).mock.results[0]?.value; - expect(loggerInstance.logInteraction).not.toHaveBeenCalled(); + expect(loggerInstance.logInteraction).toHaveBeenCalledTimes(1); + const [openaiRequest, openaiResponse, openaiError, options] = + loggerInstance.logInteraction.mock.calls[0]; + expect(openaiRequest).toEqual( + expect.objectContaining({ + model: 'test-model', + messages: [{ role: 'user', content: 'converted' }], + }), + ); + expect(openaiResponse).toEqual( + expect.objectContaining({ id: 'openai-response' }), + ); + expect(openaiError).toBeUndefined(); + expect(options).toBe(promptId); }, ); - it.each(['prompt_suggestion', 'forked_query', 'speculation'])( - 'skips logApiRequest and OpenAI logging for internal promptId %s (generateContentStream)', + it.each([ + 'prompt_suggestion', + 'forked_query', + 'speculation', + 'side-query:session-title', + ])( + 'skips logApiRequest but writes tagged OpenAI logging for internal promptId %s (generateContentStream)', async (promptId) => { const mockChunk = { responseId: 'stream-resp', @@ -1527,7 +1551,20 @@ describe('LoggingContentGenerator', () => { const loggerInstance = ( OpenAILogger as unknown as ReturnType ).mock.results[0]?.value; - expect(loggerInstance.logInteraction).not.toHaveBeenCalled(); + expect(loggerInstance.logInteraction).toHaveBeenCalledTimes(1); + const [openaiRequest, openaiResponse, openaiError, options] = + loggerInstance.logInteraction.mock.calls[0]; + expect(openaiRequest).toEqual( + expect.objectContaining({ + model: 'test-model', + messages: [{ role: 'user', content: 'converted' }], + }), + ); + expect(openaiResponse).toEqual( + expect.objectContaining({ id: 'openai-response' }), + ); + expect(openaiError).toBeUndefined(); + expect(options).toBe(promptId); }, ); }); diff --git a/packages/core/src/core/loggingContentGenerator/loggingContentGenerator.ts b/packages/core/src/core/loggingContentGenerator/loggingContentGenerator.ts index 7f15d464a1f..d229e108af3 100644 --- a/packages/core/src/core/loggingContentGenerator/loggingContentGenerator.ts +++ b/packages/core/src/core/loggingContentGenerator/loggingContentGenerator.ts @@ -216,7 +216,7 @@ export class LoggingContentGenerator implements ContentGenerator { async (span) => { const startTime = Date.now(); const isInternal = isInternalPromptId(userPromptId); - const session = this.startCaptureSession(isInternal); + const session = this.startCaptureSession(); try { if (!isInternal) { this.logApiRequest( @@ -240,18 +240,15 @@ export class LoggingContentGenerator implements ContentGenerator { response.usageMetadata, responseText, ); - if (!isInternal) { - try { - await this.safelyLogOpenAIInteraction( - await session.resolve(req), - response, - ); - } catch (loggingError) { - debugLogger.warn( - 'Failed to log OpenAI interaction:', - loggingError, - ); - } + try { + await this.safelyLogOpenAIInteraction( + await session.resolve(req), + response, + undefined, + userPromptId, + ); + } catch (loggingError) { + debugLogger.warn('Failed to log OpenAI interaction:', loggingError); } return response; } catch (error) { @@ -263,19 +260,15 @@ export class LoggingContentGenerator implements ContentGenerator { req.model, userPromptId, ); - if (!isInternal) { - try { - await this.safelyLogOpenAIInteraction( - await session.resolve(req), - undefined, - error, - ); - } catch (loggingError) { - debugLogger.warn( - 'Failed to log OpenAI interaction:', - loggingError, - ); - } + try { + await this.safelyLogOpenAIInteraction( + await session.resolve(req), + undefined, + error, + userPromptId, + ); + } catch (loggingError) { + debugLogger.warn('Failed to log OpenAI interaction:', loggingError); } safeSetStatus(span, { code: SpanStatusCode.ERROR, @@ -302,7 +295,7 @@ export class LoggingContentGenerator implements ContentGenerator { const startTime = Date.now(); const isInternal = isInternalPromptId(userPromptId); - const session = this.startCaptureSession(isInternal); + const session = this.startCaptureSession(); let stream: AsyncGenerator; try { @@ -332,22 +325,21 @@ export class LoggingContentGenerator implements ContentGenerator { } catch { // OTel errors must not mask the original API error } - if (!isInternal) { - try { - await this.safelyLogOpenAIInteraction( - await session.resolve(req), - undefined, - error, - ); - } catch (loggingError) { - debugLogger.warn('Failed to log OpenAI interaction:', loggingError); - } + try { + await this.safelyLogOpenAIInteraction( + await session.resolve(req), + undefined, + error, + userPromptId, + ); + } catch (loggingError) { + debugLogger.warn('Failed to log OpenAI interaction:', loggingError); } throw error; } let resolvedRequest: OpenAI.Chat.ChatCompletionCreateParams | undefined; - if (!isInternal) { + if (this.openaiLogger) { try { resolvedRequest = await session.resolve(req); } catch (loggingError) { @@ -368,14 +360,14 @@ export class LoggingContentGenerator implements ContentGenerator { ); } - private startCaptureSession(isInternal: boolean): { + private startCaptureSession(): { wrap: (fn: () => Promise) => Promise; resolve: ( req: GenerateContentParameters, ) => Promise; } { let captured: OpenAI.Chat.ChatCompletionCreateParams | undefined; - const skipCapture = isInternal || !this.openaiLogger; + const skipCapture = !this.openaiLogger; return { wrap: (fn: () => Promise): Promise => skipCapture @@ -384,7 +376,9 @@ export class LoggingContentGenerator implements ContentGenerator { captured = built; }, fn), resolve: async (req) => - captured ?? (await this.buildOpenAIRequestForLogging(req)), + this.openaiLogger + ? (captured ?? (await this.buildOpenAIRequestForLogging(req))) + : undefined, }; } @@ -398,8 +392,9 @@ export class LoggingContentGenerator implements ContentGenerator { spanContext?: Context, ): AsyncGenerator { const isInternal = isInternalPromptId(userPromptId); - // For internal prompts we only need the last usage metadata (for /stats); - // skip collecting full responses to avoid unnecessary memory overhead. + // Skip collecting full responses for internal prompts to avoid memory + // overhead, unless OpenAI file logging needs them. + const shouldCollectResponses = !isInternal || !!this.openaiLogger; const responses: GenerateContentResponse[] = []; // Track first-seen IDs so _logApiResponse/_logApiError have accurate @@ -423,7 +418,7 @@ export class LoggingContentGenerator implements ContentGenerator { if (!firstModelVersion && response.modelVersion) { firstModelVersion = response.modelVersion; } - if (!isInternal) { + if (shouldCollectResponses) { responses.push(response); } if (response.usageMetadata) { @@ -433,9 +428,9 @@ export class LoggingContentGenerator implements ContentGenerator { } // Only log successful API response if no error occurred const durationMs = Date.now() - startTime; - const consolidatedResponse = isInternal - ? undefined - : this.consolidateGeminiResponsesForLogging(responses); + const consolidatedResponse = shouldCollectResponses + ? this.consolidateGeminiResponsesForLogging(responses) + : undefined; runInSpan(() => this.safelyLogApiResponse( firstResponseId, @@ -443,14 +438,19 @@ export class LoggingContentGenerator implements ContentGenerator { firstModelVersion || model, userPromptId, lastUsageMetadata, - this.extractResponseText(consolidatedResponse), + isInternal + ? undefined + : this.extractResponseText(consolidatedResponse), + ), + ); + await runInSpan(() => + this.safelyLogOpenAIInteraction( + openaiRequest, + consolidatedResponse, + undefined, + userPromptId, ), ); - if (!isInternal) { - await runInSpan(() => - this.safelyLogOpenAIInteraction(openaiRequest, consolidatedResponse), - ); - } terminalStatusAttempted = true; if (span) { safeSetStatus(span, { code: SpanStatusCode.OK }); @@ -466,11 +466,14 @@ export class LoggingContentGenerator implements ContentGenerator { userPromptId, ), ); - if (!isInternal) { - await runInSpan(() => - this.safelyLogOpenAIInteraction(openaiRequest, undefined, error), - ); - } + await runInSpan(() => + this.safelyLogOpenAIInteraction( + openaiRequest, + undefined, + error, + userPromptId, + ), + ); terminalStatusAttempted = true; if (span) { safeSetStatus(span, { @@ -553,6 +556,7 @@ export class LoggingContentGenerator implements ContentGenerator { openaiRequest: OpenAI.Chat.ChatCompletionCreateParams | undefined, response?: GenerateContentResponse, error?: unknown, + promptId?: string, ): Promise { if (!this.openaiLogger || !openaiRequest) { return; @@ -570,6 +574,7 @@ export class LoggingContentGenerator implements ContentGenerator { : error ? new Error(String(error)) : undefined, + promptId, ); } @@ -577,9 +582,10 @@ export class LoggingContentGenerator implements ContentGenerator { openaiRequest: OpenAI.Chat.ChatCompletionCreateParams | undefined, response?: GenerateContentResponse, error?: unknown, + promptId?: string, ): Promise { try { - await this.logOpenAIInteraction(openaiRequest, response, error); + await this.logOpenAIInteraction(openaiRequest, response, error, promptId); } catch (loggingError) { debugLogger.warn('Failed to log OpenAI interaction:', loggingError); } diff --git a/packages/core/src/utils/internalPromptIds.ts b/packages/core/src/utils/internalPromptIds.ts index 5e49548a400..a2544540114 100644 --- a/packages/core/src/utils/internalPromptIds.ts +++ b/packages/core/src/utils/internalPromptIds.ts @@ -26,7 +26,7 @@ const SIDE_QUERY_PROMPT_PREFIX = 'side-query:'; /** * Returns true if the prompt_id belongs to an internal background operation * whose events should not be recorded to the chatRecordingService, - * OpenAI logs, or other persistent stores visible in the UI. + * telemetry payloads, or other persistent stores visible in the UI. */ export function isInternalPromptId(promptId: string | undefined): boolean { if (!promptId) return false; diff --git a/packages/core/src/utils/openaiLogger.test.ts b/packages/core/src/utils/openaiLogger.test.ts index 4aa545e104e..b93f5c73028 100644 --- a/packages/core/src/utils/openaiLogger.test.ts +++ b/packages/core/src/utils/openaiLogger.test.ts @@ -148,6 +148,53 @@ describe('OpenAILogger', () => { expect(fileExists).toBe(true); }); + it('should include sanitized internal prompt id suffix when provided', async () => { + const logger = new OpenAILogger(testTempDir); + await logger.initialize(); + + const request = { + model: 'gpt-4', + messages: [{ role: 'user', content: 'test' }], + }; + const response = { id: 'test-id', choices: [] }; + + const logPath = await logger.logInteraction( + request, + response, + undefined, + 'side-query:session-title', + ); + + expect(path.basename(logPath)).toMatch( + /openai-\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}\.\d{3}Z-[a-f0-9]{8}-side-query-session-title\.json/, + ); + + const logContent = JSON.parse(await fs.readFile(logPath, 'utf-8')); + expect(logContent).not.toHaveProperty('metadata'); + }); + + it('should not include a filename suffix for non-internal prompt ids', async () => { + const logger = new OpenAILogger(testTempDir); + await logger.initialize(); + + const request = { + model: 'gpt-4', + messages: [{ role: 'user', content: 'test' }], + }; + const response = { id: 'test-id', choices: [] }; + + const logPath = await logger.logInteraction( + request, + response, + undefined, + 'user_query', + ); + + expect(path.basename(logPath)).toMatch( + /openai-\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}\.\d{3}Z-[a-f0-9]{8}\.json/, + ); + }); + it('should write correct log data structure', async () => { const logger = new OpenAILogger(testTempDir); await logger.initialize(); diff --git a/packages/core/src/utils/openaiLogger.ts b/packages/core/src/utils/openaiLogger.ts index 43028de2c7b..374a2d343a4 100644 --- a/packages/core/src/utils/openaiLogger.ts +++ b/packages/core/src/utils/openaiLogger.ts @@ -9,9 +9,20 @@ import { promises as fs } from 'node:fs'; import { v4 as uuidv4 } from 'uuid'; import * as os from 'os'; import { createDebugLogger } from './debugLogger.js'; +import { isInternalPromptId } from './internalPromptIds.js'; const debugLogger = createDebugLogger('OPENAI_LOGGER'); +function sanitizePromptIdForFilename( + promptId: string | undefined, +): string | undefined { + if (!promptId || !isInternalPromptId(promptId)) return undefined; + const sanitized = promptId + .replace(/[^a-zA-Z0-9._-]+/g, '-') + .replace(/^-+|-+$/g, ''); + return sanitized || undefined; +} + /** * Logger specifically for OpenAI API requests and responses */ @@ -64,12 +75,15 @@ export class OpenAILogger { * @param request The request sent to OpenAI * @param response The response received from OpenAI * @param error Optional error if the request failed + * @param promptId Optional prompt id; internal prompt ids are appended to + * the filename after timestamp and id. * @returns The file path where the log was written */ async logInteraction( request: unknown, response?: unknown, error?: Error, + promptId?: string, ): Promise { if (!this.initialized) { await this.initialize(); @@ -77,7 +91,10 @@ export class OpenAILogger { const timestamp = new Date().toISOString().replace(/:/g, '-'); const id = uuidv4().slice(0, 8); - const filename = `openai-${timestamp}-${id}.json`; + const promptIdSuffix = sanitizePromptIdForFilename(promptId); + const filename = promptIdSuffix + ? `openai-${timestamp}-${id}-${promptIdSuffix}.json` + : `openai-${timestamp}-${id}.json`; const filePath = path.join(this.logDir, filename); const logData = {