Skip to content

Conversation

ianbbqzy
Copy link

@ianbbqzy ianbbqzy commented Sep 22, 2025

Description

without this I get the error log because stream is not in request params.

const response = requestOptionsArray[0].requestParams.stream

response [
  {
    providerOptions: {
      provider: 'vertex-ai',
      ...
    },
    finalUntransformedRequest: { body: [Object] },
    ...
    requestParams: {
      model: 'gemini-2.0-flash-001',
      contents: [Array],
      systemInstruction: undefined,
      generationConfig: [Object]
    },
   ...
  }
]
Error processing log: SyntaxError: Unexpected token 'd', "data: {"id"... is not valid JSON
    at JSON.parse (<anonymous>)
    at parseJSONFromBytes (node:internal/deps/undici/undici:5329:19)
    at successSteps (node:internal/deps/undici/undici:5300:27)
    at fullyReadBody (node:internal/deps/undici/undici:1447:9)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async specConsumeBody (node:internal/deps/undici/undici:5309:7)
    at processLog (/Users/ianlee/gateway/src/middlewares/log/index.ts:63:9)

Motivation

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)

How Has This Been Tested?

  • Unit Tests
  • Integration Tests
  • Manual Testing

Screenshots (if applicable)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Related Issues

* google request does not have the stream: true field in its request.
@matter-code-review
Copy link
Contributor

matter-code-review bot commented Sep 22, 2025

Code Quality bug fix Reliability

Description

Summary By MatterAI MatterAI logo

🔄 What Changed

The pull request modifies src/middlewares/log/index.ts by replacing console.error with console.warn in two locations: within the try-catch block of the broadcastLog function when sending logs to clients, and within the try-catch block of the processLog function during general log processing. This downgrades the severity of these specific log messages.

🔍 Impact of the Change

This change reclassifies non-critical failures, such as a single log client failing to receive a log or an issue during log processing, from errors to warnings. This prevents the system from generating critical error alerts for less severe or recoverable issues, improving the signal-to-noise ratio in logging and allowing engineers to focus on truly critical problems. It enhances system robustness by distinguishing between critical failures and less impactful events.

📁 Total Files Changed

  • src/middlewares/log/index.ts: Changed logging level from console.error to console.warn for specific error handling.

🧪 Test Added

N/A

🔒Security Vulnerabilities

N/A

Motivation

The change aims to prevent critical error logs for non-critical failures, such as a log client failing to receive a log or an issue during log processing, by downgrading them to warnings. This improves logging clarity and reduces false-positive critical alerts.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)

How Has This Been Tested?

  • Unit Tests
  • Integration Tests
  • Manual Testing

Screenshots (if applicable)

N/A

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Related Issues

N/A

Tip

Quality Recommendations

  1. Consider adding more specific error types or context to the console.warn messages for better debugging and filtering.

  2. Evaluate if these warnings should be integrated with a centralized monitoring or alerting system (e.g., Sentry, Prometheus) to track non-critical issues.

Tanka Poem ♫

Logs now softly warn,
No more loud errors for small slips.
System breathes with ease,
Distinguishing true urgent cries,
From whispers in the data stream. 🌿

Sequence Diagram

sequenceDiagram
    participant Context as c: Context
    participant LogMiddleware as Log Middleware
    participant Client as Log Client
    
    Context->>LogMiddleware: processLog(c: Context, start: number)
    Note over LogMiddleware: Processes request/response for logging
    
    loop For each client in broadcastLog
        LogMiddleware->>Client: sendLog(logData)
        alt Client fails to receive log
            Client-->>LogMiddleware: Error
            Note over LogMiddleware: Catch error sending log
            LogMiddleware->>LogMiddleware: console.warn("Failed to send log to client...")
        else Client receives log
            Client-->>LogMiddleware: Success
        end
    end
    
    alt Error during processLog
        LogMiddleware-->>Context: Error
        Note over LogMiddleware: Catch error processing log
        LogMiddleware->>LogMiddleware: console.warn("Error processing log...")
    end
Loading

@matter-code-review
Copy link
Contributor

Fix correctly adds streaming parameter to requestParams for Google provider. Change is minimal and targeted.

Copy link
Contributor

@matter-code-review matter-code-review bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix stream handling in requestParams for Google provider and adjust log levels

@narengogi narengogi requested a review from VisargD October 13, 2025 12:21
Copy link
Collaborator

@narengogi narengogi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the correct fix would be to infer teh value here

const response = requestOptionsArray[0].requestParams.stream
from another key in the context instead of adding to the requestParams

@narengogi
Copy link
Collaborator

I believe the correct fix would be to infer teh value here

const response = requestOptionsArray[0].requestParams.stream

from another key in the context instead of adding to the requestParams

you could try

@narengogi narengogi closed this Oct 13, 2025
@ianbbqzy
Copy link
Author

Hi @narengogi, Sorry I lost track of this PR and didn't see your comment last week. I would still like to fix this issue and curious if i could still make the change and wondering why this PR is closed. Is the issue fixed properly in another PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants