netlify / netlify-commons

MIT License
10 stars 9 forks source link

DNMY - figure out how to get reporting for endpoints with the path #325

Closed rybit closed 2 years ago

rybit commented 2 years ago

ok this is not going to be merged as is, more as a discussion.

I was working on some other service and realized I wasn't logging 401s and 404s. So I traced that back to the router. We never get a call for a 404, duuuuh I went. So I found that we have handlers for not found and method not allowed. All is good in the world.

BUT

No. So I was looking and if your middleware stack rejects the call before we get to the actual handler, so think CORS and auth, we won't log/count either. The problem is that we only wrap when we get to the very end. Then naturally we need to tweak the middleware stack to be first and then let the other handlers reject as makes sense. Sweet. Easy.

BUT

We try to encode the 'pattern' in the handler's APM for better visibility. This way we can get top level paths in datadog for GET.my.special.path and not for every unique path at the top. This is where we are now. The problem is that (1) chi doesn't know the path until after it has invoked the handler. That is after we could block the call. (2) we can't really 'register' any object I can think of or put it in the context to let us know that until after the route is defined. This means I can't easily see how often GET.my.authenticated.endpoint is getting hit with a 401. Yes we can log that now (yay) but spans won't show that.

What I'm thinking of doing is actually maybe splitting the problem. Leave the code ~as is (just the err handlers). But also start a parent span we report if we don't report another? IDK I'm pretty stumped and don't think that will actually work. Or maybe just start a new child span and put the pattern there?

rybit commented 2 years ago

Another idea I had this morning would be to let people have kinda a RouteOptions on the different builders for the routes. This would let people override the pattern nonsense here and give meaningful names. Thought it has the same limitations around the middleware as this does.

mraerino commented 2 years ago

could you use reflection to get the name of the function being registered? 😈

rybit commented 2 years ago

The problem is that the calls look like this:

  router.Route
    healthMiddleware
    .... whatever ...
    I reject things middleware
    your route handler

So if you get to the reject Middleware, you never get to the route handler. That handler is the one that currently triggers tracing and has access to the pattern (or any other marker we could). What we'd need to do to capture middleware level rejects is change it to:

router.Route
  healthMiddleware
  tracingMiddleware <-- note at the top of the chain
  ... the rest of the stuff ...

When we're at the tracing middleware, we don't know what the eventual handler is going to be. So we can't use the information that is down that low. chi has a RoutePattern field, but it is populated after the request is complete. So we can't use it on the starter span.

I'm trying to think if this is something we can tweak to more of a builder pattern (it's not far from it) so that we could record the path, but honestly it isn't solving the problem of resolving at runtime.

mraerino commented 2 years ago

maybe this helps? https://github.com/go-chi/chi/issues/270#issuecomment-479184559

rybit commented 2 years ago

related: https://github.com/netlify/identeer/issues/53