Log Errors Using Router?

[antholeole]

Hi there! I'm a fan of itty & itty extras. Unfortunately, I can't seem to find a way to log more details about 500 errors - errors that happen somewhere upstream in a router that throws. Generally, this works when I'm not using a router to log errors:

addEventListener('fetch', async (event) => {
  event.respondWith((router.handle(event.request, event) as Promise<Response>).catch((e) => {
    console.log(e)
    console.log(e.message)
    console.log(e.name)
    throw e
  }))
})

but unfortunately, the errors do not get logged - with throwable router, it seems that the errors get swallowed and returned without hitting the catch block of the promise.

My purposed solution would just be, instead of swallowing (try {...} on error { return 500 }) why not something that looks like:

try {
...
} catch (e) {
if (error is StatusError) {
 ...(handle)
else {
throw e
}
}

this will allow developers to debug the errors that they didn't intentionally throw - i.e. only StatusError and friends get handled, but actual developer errors can bubble up and be logged accordingly.

Thanks!

[kwhitley]

Hey @antholeole, glad you like the new extras! I'll be able to dive in an replicate what you’re talking about in a couple hours, but at a glance, it looks like the prob is you trying to catch with ThrowableRouter, which already catches (if you want to catch the handle yourself, just use Router from core itty). That said, a couple things on this... first of all, I’m right there with you, and in my own codebase (slick.af) I expose the stack through the error. Gotta warn you though, Worker errors (stack in particular) are often pretty useless in my experience, as everything was minified and such. I ripped it out as a quick first pass of the extras, assuming most devs wouldn't want to expose the stack trace by default, but I'll add an option in. Worst case, later tonight, I'll give you a workaround for the meantime!

[antholeole]

I actually had something very similar to ThrowableRouter before I found this package, but decided to flip to the package because I wanted less code to maintain + the extra features were very nice.

I added a proposed solution to the original issue. That proposal is essentially what I was doing before I was using ThrowableRouter.

[kwhitley]

So I'm working on this right now (and adding test coverage), and I have a couple notes on it:

• I see ThrowableRouter() as merely a one-step abstraction for those that don't need control... it will absolutely "eat" errors and respond with them automatically, based on the options - as that's what it was designed for; saving you from sloppy code and unintentional throws, without having to write the catch yourself. I'm testing adding a { stack: false|true } option to ThrowableRouter() that automatically exposes the stack (if given) to the payload if asked for, but again... this is the lazy version of error-handling for the masses. Regardless, exposing the stack will definitely be off by default, as that should be a debug-only mode (opt-in) that carries some risk of exposing internal code :)

    // this is using ES module syntax for CF
    import { ThrowableRouter } from 'itty-router-extras'
    
    const router = ThrowableRouter({ stack: true })
    
    exports default {
      fetch: router.handle // yep, that's it
    }

• It sounds like you'd honestly be better off with just the itty core Router if you want your own error handling (ThrowableRouter already catches and responds before your catch is ever seen)

    // this is using ES module syntax for CF
    import { Router } from 'itty-router'
    import { error } from 'itty-router-extras'
    
    const router = Router()
    
    exports default {
      fetch: (...args) => router.handle(...args).catch(err => {
        // do some other stuff with the error
        
        // err.status will only exist if a `StatusError` was thrown
        return error(err.status || 500, {
          message: err.message,
          stack: err.stack,
        })
      })
    }

  • I'm adding an overload to error(status: number, content:string|object) to handle this (and other) cases. If you pass in an object as 2nd param, it'll explode into the payload, otherwise will be simply: { error: content }

    error(400, 'Bad Request') 
    // { status: 400, error: 'Bad Request' }
    
    error(400, { message: 'Bad Request', details: 'It was really bad.' })
    // { status: 400, message: 'Bad Request', details: 'It was really bad.' }

Thoughts?

[kwhitley]

Also, since you use both (itty and extras), I'd love your opinion on something... I've obviously been using both throughout my own code, but the README examples on itty core don't currently use extras (they weren't published, plus I wanted to make it readable for people that only wanted the core router for whatever reason). Think I should include extras in the examples?

That's obviously when the code gets really tiny...

[antholeole]

I like your solution, but what happens in the case that a user wants to use something like sentry for remote error logging? In this case, using a throwable router is impossible - no exceptions will ever make it up to the sentry catcher.

Regardless, thanks for your work on this package - the utils are great and with or without Throwable Router and I'll definitely continue to use it.

Per your question - I think how you have it now is nice, with a callout to itty-router-extras in the itty-router README. Helps with discoverability.

When I look at a readme I usually laser lock on my use-case (command f what I need) and if an example pops up that uses another package, I may try to copy and paste that directly into my code and wonder why something like StatusError isn't importing.

So my vote goes no - don't include the itty-router-extras examples on itty-core as I think your callout in the itty-router to itty-router-extras does the job.

[kwhitley]

Surely something like this would work?

without async event.waitUntil after response (easier)

import { Router } from 'itty-router'
import { error } from 'itty-router-extras'
import { Sentry } from 'borderless/worker-sentry'

const sentry = new Sentry({ dsn: 'https://123@456.ingest.sentry.io/789' })
const router = Router() // not ThrowableRouter which will respond automatically to throws
        
addEventListener('fetch', event => {
  event.respondWith(
    router
      // attempt to handle route
      .handle(event.request, event)
      
      // return/respond to request with sentry response promise
      .catch(err => sentry.captureException(err, {
        tags: {},
        user: {
          ip_address: event.request.headers.get("cf-connecting-ip"),
        },
      })
      
      // respond with final catch-all in case sentry logging throws somehow
      .catch(err => error(500, { 
        error: 'Could not log error to Sentry'
        details: err.message,
        stack: err.stack,
      })
  )
})

or if you want to fire of the logger after the request resolves to speed up response times (what they attempt to show in their docs I believe), just replace the sentry block with this:

.catch(err => {
  // fire off the logger promise
  event.waitUntil(sentry.captureException(err, {
    tags: {},
    user: {
      ip_address: event.request.headers.get("cf-connecting-ip"),
    }
  })
  
  // then respond to the actual request immediately
  return error(500, 'Logging error to sentry')
})

[antholeole]

Yep! Those would work perfectly fine! I'm just saying that it has incompatibility with the ThrowableRouter. That being said, I know the compatibility with bubble-up errors isn't what you're going for with ThrowableRouter - I'll take your advice and switch to the core router with custom error handling.

Thanks for all the help! Feel free to close the issue; I'll leave it open just in case you want to make changes based on it and want to create a PR.

[kwhitley]

Ok cool, we'll I’m def expanding the extras API a bit to have more flexible errors (per the convo above), but yeah... ThrowableRouter is the broad stroke easy path for people with nothing elaborate in their flow. I want to get loads of people into the Workers ecosystem so we can see crazy cool stuff, so I’m just trying to make things look easy as a starting point.

And then for my rapid dev projects/experiments, I never bother with incredibly useful things like error logging... because you know, time and such ;) I'll close this once the update is released to remind me that I've unfinished business to attend to!

Thanks for the feedback/discussion!