From 2c5e09e1c300825449ec6c8be172da6ae3d87bd5 Mon Sep 17 00:00:00 2001 From: Tommaso Sciortino Date: Thu, 13 Nov 2025 19:11:13 -0800 Subject: [PATCH] Update comment and undo unnecessary logging (#13025) --- packages/core/src/core/geminiChat.ts | 9 ++++ packages/core/src/telemetry/loggers.ts | 4 +- packages/core/src/telemetry/metrics.test.ts | 50 --------------------- packages/core/src/telemetry/metrics.ts | 28 ++++-------- 4 files changed, 20 insertions(+), 71 deletions(-) diff --git a/packages/core/src/core/geminiChat.ts b/packages/core/src/core/geminiChat.ts index 9939602b3b..cf3b0df50c 100644 --- a/packages/core/src/core/geminiChat.ts +++ b/packages/core/src/core/geminiChat.ts @@ -575,6 +575,15 @@ export class GeminiChat { content: responseText, }); } + + // Stream validation logic: A stream is considered successful if: + // 1. There's a tool call OR + // 2. A not MALFORMED_FUNCTION_CALL finish reason and a non-mepty resp + // + // We throw an error only when there's no tool call AND: + // - No finish reason, OR + // - MALFORMED_FUNCTION_CALL finish reason OR + // - Empty response text (e.g., only thoughts with no actual content) if (!hasToolCall) { if (!finishReason) { throw new InvalidStreamError( diff --git a/packages/core/src/telemetry/loggers.ts b/packages/core/src/telemetry/loggers.ts index aef1d32734..a84770c096 100644 --- a/packages/core/src/telemetry/loggers.ts +++ b/packages/core/src/telemetry/loggers.ts @@ -442,7 +442,7 @@ export function logContentRetry( attributes: event.toOpenTelemetryAttributes(config), }; logger.emit(logRecord); - recordContentRetry(config, event.error_type); + recordContentRetry(config); } export function logContentRetryFailure( @@ -458,7 +458,7 @@ export function logContentRetryFailure( attributes: event.toOpenTelemetryAttributes(config), }; logger.emit(logRecord); - recordContentRetryFailure(config, event.final_error_type); + recordContentRetryFailure(config); } export function logModelRouting( diff --git a/packages/core/src/telemetry/metrics.test.ts b/packages/core/src/telemetry/metrics.test.ts index 836baaf7d6..dbb231f7cf 100644 --- a/packages/core/src/telemetry/metrics.test.ts +++ b/packages/core/src/telemetry/metrics.test.ts @@ -96,8 +96,6 @@ describe('Telemetry Metrics', () => { let recordAgentRunMetricsModule: typeof import('./metrics.js').recordAgentRunMetrics; let recordLinesChangedModule: typeof import('./metrics.js').recordLinesChanged; let recordSlowRenderModule: typeof import('./metrics.js').recordSlowRender; - let recordContentRetryModule: typeof import('./metrics.js').recordContentRetry; - let recordContentRetryFailureModule: typeof import('./metrics.js').recordContentRetryFailure; beforeEach(async () => { vi.resetModules(); @@ -142,8 +140,6 @@ describe('Telemetry Metrics', () => { recordAgentRunMetricsModule = metricsJsModule.recordAgentRunMetrics; recordLinesChangedModule = metricsJsModule.recordLinesChanged; recordSlowRenderModule = metricsJsModule.recordSlowRender; - recordContentRetryModule = metricsJsModule.recordContentRetry; - recordContentRetryFailureModule = metricsJsModule.recordContentRetryFailure; const otelApiModule = await import('@opentelemetry/api'); @@ -1347,50 +1343,4 @@ describe('Telemetry Metrics', () => { }); }); }); - - describe('recordContentRetry', () => { - it('does not record metrics if not initialized', () => { - const config = makeFakeConfig({}); - recordContentRetryModule(config, 'NO_FINISH_REASON'); - expect(mockCounterAddFn).not.toHaveBeenCalled(); - }); - - it('records a content retry event with error type when initialized', () => { - const config = makeFakeConfig({}); - initializeMetricsModule(config); - mockCounterAddFn.mockClear(); // Clear the session start call - - recordContentRetryModule(config, 'MALFORMED_FUNCTION_CALL'); - - expect(mockCounterAddFn).toHaveBeenCalledWith(1, { - 'session.id': 'test-session-id', - 'installation.id': 'test-installation-id', - 'user.email': 'test@example.com', - error_type: 'MALFORMED_FUNCTION_CALL', - }); - }); - }); - - describe('recordContentRetryFailure', () => { - it('does not record metrics if not initialized', () => { - const config = makeFakeConfig({}); - recordContentRetryFailureModule(config, 'NO_RESPONSE_TEXT'); - expect(mockCounterAddFn).not.toHaveBeenCalled(); - }); - - it('records a content retry failure event with error type when initialized', () => { - const config = makeFakeConfig({}); - initializeMetricsModule(config); - mockCounterAddFn.mockClear(); // Clear the session start call - - recordContentRetryFailureModule(config, 'MALFORMED_FUNCTION_CALL'); - - expect(mockCounterAddFn).toHaveBeenCalledWith(1, { - 'session.id': 'test-session-id', - 'installation.id': 'test-installation-id', - 'user.email': 'test@example.com', - error_type: 'MALFORMED_FUNCTION_CALL', - }); - }); - }); }); diff --git a/packages/core/src/telemetry/metrics.ts b/packages/core/src/telemetry/metrics.ts index a0d2267c7b..f2078aa62d 100644 --- a/packages/core/src/telemetry/metrics.ts +++ b/packages/core/src/telemetry/metrics.ts @@ -136,17 +136,13 @@ const COUNTER_DEFINITIONS = { description: 'Counts retries due to content errors (e.g., empty stream).', valueType: ValueType.INT, assign: (c: Counter) => (contentRetryCounter = c), - attributes: {} as { - error_type: string; - }, + attributes: {} as Record, }, [CONTENT_RETRY_FAILURE_COUNT]: { description: 'Counts occurrences of all content retries failing.', valueType: ValueType.INT, assign: (c: Counter) => (contentRetryFailureCounter = c), - attributes: {} as { - error_type: string; - }, + attributes: {} as Record, }, [MODEL_ROUTING_FAILURE_COUNT]: { description: 'Counts model routing failures.', @@ -719,26 +715,20 @@ export function recordInvalidChunk(config: Config): void { /** * Records a metric for when a retry is triggered due to a content error. */ -export function recordContentRetry(config: Config, errorType: string): void { +export function recordContentRetry(config: Config): void { if (!contentRetryCounter || !isMetricsInitialized) return; - contentRetryCounter.add(1, { - ...baseMetricDefinition.getCommonAttributes(config), - error_type: errorType, - }); + contentRetryCounter.add(1, baseMetricDefinition.getCommonAttributes(config)); } /** * Records a metric for when all content error retries have failed for a request. */ -export function recordContentRetryFailure( - config: Config, - errorType: string, -): void { +export function recordContentRetryFailure(config: Config): void { if (!contentRetryFailureCounter || !isMetricsInitialized) return; - contentRetryFailureCounter.add(1, { - ...baseMetricDefinition.getCommonAttributes(config), - error_type: errorType, - }); + contentRetryFailureCounter.add( + 1, + baseMetricDefinition.getCommonAttributes(config), + ); } export function recordModelSlashCommand(