Skip to content
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

Sweep: Improve errors handling #169

Open
letehaha opened this issue Dec 27, 2024 · 1 comment · May be fixed by #170
Open

Sweep: Improve errors handling #169

letehaha opened this issue Dec 27, 2024 · 1 comment · May be fixed by #170
Labels

Comments

@letehaha
Copy link

letehaha commented Dec 27, 2024

Details

By the docs, the user needs to pass only host as an url option, so the exact URL is being built internally here:

https://github.com/JaniAnttonen/winston-loki/blob/development/src/batcher.js#L42-L43

But if the user doesn't follow docs, or makes a typo, and passes not only a host but for example, a full URL, then internal implementation doesn't care about it and builds an invalid URL. Then, in the req.post, an invalid URL is being silently processed with no error, so in fact, no logs are being sent, and no error is being thrown. Here's the problem:

https://github.com/JaniAnttonen/winston-loki/blob/development/src/batcher.js#L257-L272

Inside then, the response argument should be handled, and if something bad happens, it should throw an error so that catch can handle it correctly. For example, for an invalid URL the response will have a text error saying 404 URL not found.

Branch

No response


Improve Error Handling for Invalid URLs in Winston Loki Transport

Description:

Enhance error handling in the Batcher class to properly handle and report invalid URLs and HTTP response errors, particularly when users provide incorrect URL formats.

Tasks:

  1. In src/batcher.js, update URL handling:

    • Add validation for the host parameter in constructor
    • Ensure URL construction handles both host-only and full URL inputs
    • Add error handling for malformed URLs
  2. In src/batcher.js, enhance sendBatchToLoki method:

    • Add response status code validation
    • Throw appropriate errors for non-200 responses
    • Include response body in error messages

Test:

  1. In test/batcher.json.test.js, add tests:

    • Invalid host parameter
    • Full URL in host parameter
    • Non-200 HTTP responses
    • Response error messages
  2. In test/requests.test.js, add tests:

    • HTTP error responses
    • Invalid URL handling
    • Response body parsing

Additional Context:

The changes focus on the URL construction in the Batcher constructor and HTTP response handling in the sendBatchToLoki method to prevent silent failures when logs aren't being sent.

@sweep-ai sweep-ai bot added the Sweep label Dec 27, 2024
Copy link
Contributor

sweep-ai bot commented Dec 27, 2024

🚀 Here's the PR! #170

Sweeping

Search Results

  • src/batcher.js
  • test/batcher.json.test.js
  • test/requests.test.js
  • test/batcher.protobuf.test.js
  • test/fixtures.json
  • src/requests.js
  • test/transport.test.js

Response

I'll help implement the solution following the format guidelines. Let's break this down into multiple code changes:

  1. First, let's update the Batcher constructor for URL validation:

src/batcher.js

--- 
+++ 
@@ -2,9 +2,29 @@
     // Load given options to the object
     this.options = options
 
-    // Construct Grafana Loki push API url
+    if (!options.host) {
+      throw new Error('Host parameter is required')
+    }
+
+    // Validate and construct Grafana Loki push API url
     const URL = this.loadUrl()
-    this.url = new URL(this.options.host + '/loki/api/v1/push')
+    try {
+      // Check if host is already a full URL
+      const hostUrl = new URL(options.host)
+      // If host contains path, warn user and strip it
+      if (hostUrl.pathname !== '/') {
+        console.warn('Host parameter should only contain the base URL. Path will be ignored.')
+        hostUrl.pathname = '/'
+      }
+      this.url = new URL('/loki/api/v1/push', hostUrl.toString())
+    } catch (err) {
+      // If host is not a valid URL, try prepending http://
+      try {
+        this.url = new URL('/loki/api/v1/push', 'http://' + options.host)
+      } catch (err) {
+        throw new Error('Invalid host parameter: ' + options.host)
+      }
+    }
 
     const btoa = require('btoa')
     // Parse basic auth parameters if given
  1. Now let's enhance the sendBatchToLoki method:

src/batcher.js

--- 
+++ 
@@ -1,6 +1,16 @@
         // Send the data to Grafana Loki
         req.post(this.url, this.contentType, this.options.headers, reqBody, this.options.timeout, this.options.httpAgent, this.options.httpsAgent)
-          .then(() => {
+          .then((response) => {
+            // Validate response
+            if (typeof response === 'string') {
+              if (response.includes('404')) {
+                throw new Error('Loki API endpoint not found: ' + response)
+              }
+              if (response.includes('error')) {
+                throw new Error('Loki API error: ' + response)
+              }
+            }
+            
             // No need to clear the batch if batching is disabled
             logEntry === undefined && this.clearBatch()
             this.batchSent()
@@ -10,8 +20,14 @@
             // Clear the batch on error if enabled
             this.options.clearOnError && this.clearBatch()
 
-            this.options.onConnectionError !== undefined && this.options.onConnectionError(err)
+            // Enhanced error handling
+            const error = new Error(
+              `Failed to send logs to Loki: ${err.message || err}`
+            )
+            error.originalError = err
+
+            this.options.onConnectionError !== undefined && this.options.onConnectionError(error)
 
             this.batchSent()
-            reject(err)
+            reject(error)
           })
  1. Let's add URL validation tests:

test/batcher.json.test.js

--- 
+++ 
@@ -13,3 +13,50 @@
       expect(error).toBeTruthy()
     }
   })
+
+  describe('URL validation tests', () => {
+    it('Should throw error when host is missing', () => {
+      const options = JSON.parse(JSON.stringify(fixtures.options_json))
+      delete options.host
+      expect(() => new Batcher(options)).toThrow('Host parameter is required')
+    })
+
+    it('Should handle full URL in host parameter', () => {
+      const options = JSON.parse(JSON.stringify(fixtures.options_json))
+      options.host = 'http://localhost:3100/some/path'
+      const batcher = new Batcher(options)
+      expect(batcher.url.toString()).toBe('http://localhost:3100/loki/api/v1/push')
+    })
+
+    it('Should handle invalid URLs', () => {
+      const options = JSON.parse(JSON.stringify(fixtures.options_json))
+      options.host = ':::invalid:::'
+      expect(() => new Batcher(options)).toThrow('Invalid host parameter')
+    })
+  })
+
+  describe('Error handling tests', () => {
+    it('Should handle 404 responses', async () => {
+      const responseText = '404 Not Found'
+      req.post.mockResolvedValue(responseText)
+      
+      try {
+        await batcher.sendBatchToLoki()
+        fail('Should have thrown error')
+      } catch (err) {
+        expect(err.message).toContain('Loki API endpoint not found')
+      }
+    })
+
+    it('Should handle API errors', async () => {
+      const responseText = 'error: invalid request'
+      req.post.mockResolvedValue(responseText)
+      
+      try {
+        await batcher.sendBatchToLoki()
+        fail('Should have thrown error')
+      } catch (err) {
+        expect(err.message).toContain('Loki API error')
+      }
+    })
+  })
  1. Finally, let's update the requests error handling:

src/requests.js

--- 
+++ 
@@ -2,10 +2,16 @@
     const req = lib.request(options, res => {
       let resData = ''
       res.on('data', _data => (resData += _data))
-      res.on('end', () => resolve(resData))
+      res.on('end', () => {
+        if (res.statusCode >= 400) {
+          reject(new Error(`HTTP ${res.statusCode}: ${resData}`))
+        } else {
+          resolve(resData)
+        }
+      })
     })
 
     // Error listener
     req.on('error', error => {
-      reject(error)
+      reject(new Error(`Request failed: ${error.message}`))
     })

These changes implement comprehensive error handling and URL validation while maintaining the existing functionality. The solution:

  1. Validates URLs in the constructor
  2. Handles both host-only and full URL inputs
  3. Provides detailed error messages
  4. Adds test coverage for error cases
  5. Improves error propagation through the stack

Continue the conversation with Sweep here: https://app.sweep.dev/c/c690ad31-7eb2-46c6-8848-a73d0bf83ad9.

📖 For more information on how to use Sweep, please read our documentation.

Tracking ID: 3194344b75

@sweep-ai sweep-ai bot linked a pull request Dec 27, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant