Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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<typeof vi.fn>
).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',
Expand Down Expand Up @@ -1527,7 +1551,20 @@ describe('LoggingContentGenerator', () => {
const loggerInstance = (
OpenAILogger as unknown as ReturnType<typeof vi.fn>
).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);
},
);
});
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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) {
Expand All @@ -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,
Expand All @@ -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<GenerateContentResponse>;
try {
Expand Down Expand Up @@ -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) {
Expand All @@ -368,14 +360,14 @@ export class LoggingContentGenerator implements ContentGenerator {
);
}

private startCaptureSession(isInternal: boolean): {
private startCaptureSession(): {
wrap: <T>(fn: () => Promise<T>) => Promise<T>;
resolve: (
req: GenerateContentParameters,
) => Promise<OpenAI.Chat.ChatCompletionCreateParams | undefined>;
} {
let captured: OpenAI.Chat.ChatCompletionCreateParams | undefined;
const skipCapture = isInternal || !this.openaiLogger;
const skipCapture = !this.openaiLogger;
return {
wrap: <T>(fn: () => Promise<T>): Promise<T> =>
skipCapture
Expand All @@ -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,
};
}

Expand All @@ -398,8 +392,9 @@ export class LoggingContentGenerator implements ContentGenerator {
spanContext?: Context,
): AsyncGenerator<GenerateContentResponse> {
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
Expand All @@ -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) {
Expand All @@ -433,24 +428,29 @@ 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,
durationMs,
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 });
Expand All @@ -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, {
Expand Down Expand Up @@ -553,6 +556,7 @@ export class LoggingContentGenerator implements ContentGenerator {
openaiRequest: OpenAI.Chat.ChatCompletionCreateParams | undefined,
response?: GenerateContentResponse,
error?: unknown,
promptId?: string,
): Promise<void> {
if (!this.openaiLogger || !openaiRequest) {
return;
Expand All @@ -570,16 +574,18 @@ export class LoggingContentGenerator implements ContentGenerator {
: error
? new Error(String(error))
: undefined,
promptId,
);
}

private async safelyLogOpenAIInteraction(
openaiRequest: OpenAI.Chat.ChatCompletionCreateParams | undefined,
response?: GenerateContentResponse,
error?: unknown,
promptId?: string,
): Promise<void> {
try {
await this.logOpenAIInteraction(openaiRequest, response, error);
await this.logOpenAIInteraction(openaiRequest, response, error, promptId);
} catch (loggingError) {
debugLogger.warn('Failed to log OpenAI interaction:', loggingError);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/utils/internalPromptIds.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
47 changes: 47 additions & 0 deletions packages/core/src/utils/openaiLogger.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Loading
Loading