gobuffalo / buffalo

Rapid Web Development w/ Go
http://gobuffalo.io
MIT License
8.08k stars 575 forks source link

Access to set ResponseWriter in buffalo.Context #1997

Closed saurori closed 3 years ago

saurori commented 4 years ago

Description

When trying to implement the New Relic v3 Agent, I realized the buffalo.Context does not provide access to the ResponseWriter. This is necessary for setting the response on the New Relic transaction via SetWebResponse to instrument response code and response headers.

Other routers/frameworks provide access, such as gin via Writer and echo via SetResponse.

Can the Context provide access to ResponseWriter, unless I am missing something with Buffalo Context implementation and how to integrate with New Relic Agent v3?

Info

paganotoni commented 4 years ago

Hey @saurori 👋. Thanks for this question. I didn't know there was a new version of the new relic go agent. I think you can implement this through a middleware.

func NewRelicV3Middleware(next buffalo.Handler) buffalo.Handler {
    return func(c buffalo.Context) error {
        // Do here the NR configuration and tracing
        // Here you should have access to the request and response from the context c.
    }
}
saurori commented 4 years ago

Yes that is what I am trying to do. I currently have it implemented as Middleware. But since there is no ability to set the ResponseWriter, response codes cannot be captured. Example (where na is a NewRelic Application:

app.Use(func(next buffalo.Handler) buffalo.Handler {
    return func(c buffalo.Context) error {
        req := c.Request()
        txn := na.StartTransaction(req.URL.String())
        defer txn.End()
        txn.SetWebRequestHTTP(req)
        ri := c.Value("current_route").(buffalo.RouteInfo)
        txn.AddAttribute("PathName", ri.PathName)
        txn.AddAttribute("RequestID", c.Value("request_id"))

        // NOTE: w cannot be used, no ability to set ResponseWriter on buffalo.Context
        // w := txn.SetWebResponse(c.Response())

        err := next(c)
        if err != nil {
            txn.NoticeError(err)
            return err
        }

        return nil
    }
})
paganotoni commented 4 years ago

Right, You're right, there is no current way to set the response on the context in buffalo. I think if you set the response after the next(c) it will have the response code, the issue with how you have it now is that the handler has not been executed when you call txn.SetWebResponse(...).

Something like:


app.Use(func(next buffalo.Handler) buffalo.Handler {
    return func(c buffalo.Context) error {
        req := c.Request()
        txn := na.StartTransaction(req.URL.String())
        defer txn.End()
        txn.SetWebRequestHTTP(req)
        ri := c.Value("current_route").(buffalo.RouteInfo)
        txn.AddAttribute("PathName", ri.PathName)
        txn.AddAttribute("RequestID", c.Value("request_id"))

        err := next(c)
        if err != nil {
            txn.NoticeError(err)
            return err
        }

                // Notice that I set it after the handlers have been executed.
                txn.SetWebResponse(c.Response())
        return nil
    }
})
saurori commented 4 years ago

I tried that as well, with no luck. After using that middleware, I triggered a 404 on my site and expect to see that show up under Transactions / Errors in NewRelic.

NewRelic go-agent docs for SetWebResponse states:

// SetWebResponse allows the Transaction to instrument response code and
// response headers.  Use the return value of this method in place of the input
// parameter http.ResponseWriter in your instrumentation.

// The returned http.ResponseWriter implements the combination of
// http.CloseNotifier, http.Flusher, http.Hijacker, and io.ReaderFrom
// implemented by the input http.ResponseWriter.

Also it is expected that a 4xx response is set as Error:

Errors are automatically recorded when
// Transaction.WriteHeader receives a status code at or above 400 or strictly
// below 100 that is not in the IgnoreStatusCodes configuration list.  This
// method is unaffected by the IgnoreStatusCodes configuration list.

This does not work because the NewRelic ResponseWriter is supposed to act as a man-in-the-middle of the response and write the headers as the http response is written. Calling SetWebResponse after next(c) means the response code and headers are no intercepted.

saurori commented 4 years ago

I was able to track down why 404 data was not making it to New Relic. It turns out that hitting a non-existing route such as https://example.com/route-does-not-exist will fall through to the app.ServeFiles() as File Not Found here. ServeFiles does not call Middleware, so the New Relic transaction was never created. I am not sure if this was intentional.

So the above Middleware I created works for 5xx errors or for 404 errors only where a route matches but returns error such as app.GET("/foo/{id}") where the Handler returns err on model not found.

For sending proper response code to New Relic, I was able to work around this but it's a bit of a hack involving setting the new relic transaction on the buffalo.Context and setting the response code in a custom error handler, so:

app.Use(func(next buffalo.Handler) buffalo.Handler {
    return func(c buffalo.Context) error {
        req := c.Request()
        txn := na.StartTransaction(req.URL.String())

        txn.SetWebRequestHTTP(req)
        ri := c.Value("current_route").(buffalo.RouteInfo)
        txn.AddAttribute("PathName", ri.PathName)
        txn.AddAttribute("RequestID", c.Value("request_id"))

        c.Set("nr_txn", txn)

        if err := next(c); err != nil {
            return err
        }
        // Only end transaction if no error. Error handlers will end transactions
        txn.End()

        return nil
    }
})

app.ErrorHandlers[http.StatusNotFound] = func(status int, origErr error, c buffalo.Context) error {
    c.Logger().WithField("status", status).Error(origErr)
    if txn, ok := c.Value("nr_txn").(*newrelic.Transaction); ok {
        txn.NoticeError(origErr)
        txn.AddAttribute("httpResponseCode", status)
        defer txn.End()
    }
    c.Response().WriteHeader(status)
    c.Response().Header().Set("content-type", defaultErrorCT)
    if _, err := c.Response().Write([]byte(prodNotFoundTmpl)); err != nil {
        return err
    }
    return nil
}

This is less than ideal, but it's the only way to get the response code since the error handler is called after Middleware. The downside here is that you have to implement custom error handlers for every single error type if you want to ensure the httpResponseCode is set and txn.End() is called.

github-actions[bot] commented 3 years ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

github-actions[bot] commented 3 years ago

This issue was closed because it has been stalled for 5 days with no activity.

ddouglas commented 2 years ago

Hey Man, not sure if this still an issue or if you have time to go back and clean up the hack you implemented(your words), but buffalo exposes the mux.Router(). You can hook into that an set up a "proper" middleware with access to the request and response from the http.Handler

Obviously I am going to clean this up and move all into the function I am calling so I can just do router.Use(s.PageCacher) but you get the point

        // s.app is an instance of *buffalo.App
    router := s.app.Muxer()
    router.Use(mux.MiddlewareFunc(func(h http.Handler) http.Handler {
        return s.PageCacher(h)
    }))
saurori commented 2 years ago

@ddouglas Thanks for the heads up, that is useful to know. However, since the app.ServeFiles() does not call middleware, there is still the problem of unmatched routes that would not be sent to New Relic. The above solution has been working for me, albeit not the most elegant.

ddouglas commented 2 years ago

@saurori This implementation is executed prior to the buffalo evaluating the request. I logged the request.URL for the homepage of a site I'm working on right now:

URL: /users/settings/
....omitted buffalo app log here.
URL: /robots.txt/?1642891980604
URL: /assets/application.css/
URL: /assets/logo.png/
URL: /assets/favicon/favicon-32x32.png/