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

Middleware incompatible with NewRelic #10

Closed
kevburnsjr opened this issue Sep 24, 2019 · 1 comment

Comments

@kevburnsjr
Copy link
Owner

@kevburnsjr kevburnsjr commented Sep 24, 2019

Putting this here because it's come up in real world situations and I don't yet feel I have a perfect solution.


Many projects use NewRelic's go agent to instrument their APIs.

The agent works as http middleware, wrapping the response writer as a newrelic.Transaction passed to the next HTTP handler.

Microcache works by creating an http.ResponseRecorder and passing that to the next HTTP middleware.

If NewRelic is above Microcache in the middleware stack, code inside a downstream handler which attempts to typecast the ResponseWriter to newrelic.Transaction will fail. This prevents controllers from appending useful information to transactions, such as database segments and custom attributes.

The only solution I can think of is to place NewRelic below Microcache in the middleware stack. However, this means that only MISSes will be recorded in NewRelic. This is not a complete solution.

Is the NewRelic agent violating some design principle that's causing this incompatibility?
Or is Microcache the one to blame?

The best solution I've devised involves 2 newrelic transactions

# Example middleware stack in order of execution

  [ New Relic Transaction Middleware A ]  (records all cached responses)
> [ New Relic Ignore Middleware]          (tells New Relic A to ignore uncached responses)
  [ Microcache ]
  [ New Relic Transaction Middleware B ]  (records all uncached responses)

Here's what that middleware might look like:

package microcachehelper

import (
	"net/http"
	
	"github.com/newrelic/go-agent"
)

func NewRelicIgnoreMiddleware(h http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		h.ServeHTTP(w, r)
		if _, ok := w.(newrelic.Transaction); !ok {
			return
		}
		status := w.Header().Get("Microcache")
		if status != "HIT" && status != "STALE" && len(status) > 0 {
			w.(newrelic.Transaction).Ignore()
		}
	})
}

Obviously I don't want to make newrelic a dependency for the microcache repository, so I would probably create a new repo for this middleware.

Creating two transactions rather than one is clearly less efficient, but the overhead is probably minimal since stats for ignored transactions don't need to be flushed to NewRelic.

Placing NewRelic below Microcache for misses may obscure latency contributed to the request by microcache. If collapsed forwarding hits an edge case or the LRU cache exceeds the machine's memory capacity and begins to swap causing increased latency on the API, we'd want that information to be visible.

Do you think this is a problem worth addressing?
Has anyone had similar problems with other HTTP middleware?
Is there a better technical solution to this problem?
Or is this just a thing that should be documented?
Or does the architecture of the project need to change to prevent these types of issues?

@kevburnsjr kevburnsjr changed the title Middleware breaks NewRelic integration Middleware incompatible with NewRelic Sep 25, 2019
@kevburnsjr

This comment has been minimized.

Copy link
Owner Author

@kevburnsjr kevburnsjr commented Sep 25, 2019

I believe I've formed a strong opinion on this matter.


Any method that accepts an argument identified by an interface should rely only on the methods provided by that interface. Encouraging users to type cast http.ResponseWriter to newrelic.Transaction in their HTTP handlers equates to poor design.

The L in SOLID refers to the Liskov Substitution Principle. New Relic's Go Agent documentation and examples repeatedly encourage users to violate this principle as the primary means of accessing the transaction within a handler.

Specifically, the constraint that preconditions cannot be strengthened in a subtype does not hold true for this http.Handler since every ResponseWriter must also be a Transaction in order for the function to operate correctly.

func myHandler(w http.ResponseWriter, r *http.Request) {
    if txn, ok := w.(newrelic.Transaction); ok {                   // <-- Incorrect
        txn.NoticeError(errors.New("my error message"))
    }
}

The better solution is to retrieve the transaction from the request context with the provided method newrelic.FromContext

func myHandler(w http.ResponseWriter, r *http.Request) {
    if txn := newrelic.FromContext(r.Context()); txn != nil {     // <-- Correct
        txn.NoticeError(errors.New("my error message"))
    }
}

Modifying your code to retrieve the newrelic transaction from the request context in this way will eliminate the above described incompatibility and probably protect you from a whole class of similar problems related to HTTP middleware.

newrelic.WrapHandle and nrgorilla.InstrumentRoutes add the transaction to the request context by default (since 2.0.0) so it's probably already there in your code, waiting to be used.


tl;dr - Refactor all your code to use newrelic.FromContext

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.