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

Gin Framework Support #28

Closed
wants to merge 3 commits into from
Closed

Gin Framework Support #28

wants to merge 3 commits into from

Conversation

willnewrelic
Copy link

WARNING: THIS CODE IS IN DEVELOPMENT. IT IS NOT A FINAL NEW RELIC PRODUCT OR OFFERING. DO NOT USE IN PRODUCTION. USE AT YOUR OWN RISK. TERMS AND CONDITIONS APPLY - SEE LICENSE FILE.

Hi All

This is pre-release support for the Gin framework. Let us know what you think. We are eager to get your feedback!

@willnewrelic
Copy link
Author

Is the v1 subdirectory the best way to handle framework versioning?

@willnewrelic
Copy link
Author

Should the Transaction be added to the gin Context so that it can be accessed in the handlers?

@jasonab
Copy link

jasonab commented Jan 19, 2017

I would love to see some error integration into this middleware, so that if I do AbortWithError, that is automatically picked up by the Gin integration and flashed to NR.

@rodriguezgustavo
Copy link

Hi @willnewrelic . We using your middleware. It works great. As you mention in your comment, it would be very useful to have the transaction available in the handlers. Is it possible to add it to the gin.Context? Without the transaction we cannot show segments and other useful stuff. On the other hand, do you now when it would be released? Thanks

@willnewrelic
Copy link
Author

@rodriguezgustavo Just added a new commit that adds a function nrgin.Transaction to access the transaction. Enjoy! Example use:

func handlerAccessTransaction(c *gin.Context) {
	if txn := nrgin.Transaction(c); nil != txn {
		txn.SetName("custom-name")
	}
	c.Writer.WriteString("changed the name of the transaction!")
}

@jasonab Interesting idea. Upon investigating, I was surprised to see that using AbortWithError does not automatically record errors when the code is greater than 400 (and not 404). It should be intercepted... Looks like the gin.Context.Status function uses Context.writermem and not Context.Writer, which I don't understand. I will make a PR to gin to see what they think.

@willnewrelic
Copy link
Author

Looks like someone already made a (broken) PR to gin to fix Context.Status: gin-gonic/gin#625

@ingshtrom
Copy link

What is the status of this? How does this get in and then also how does it get "approved" for production use?

@rodriguezgustavo
Copy link

HI, we were using this code in production for more than 1 month without any issue (APIs with more than 1 million RPM). Do you know when will you merge this PR to master?

@LeoAdamek
Copy link

I'm also very interested in when this integration will become generally available. It sounds like its pretty stable according to @rodriguezgustavo , I'd be willing to try this pre-release out on a pre-release app but I'm unable to get the integration to pull into my vendor dir so can't try it out.

@ingshtrom
Copy link

@rodriguezgustavo could you elaborate on how you are running this? How did you set this up with a basic setup of gin? tia

@LeoAdamek
Copy link

LeoAdamek commented Apr 10, 2017

I don't know how @rodriguezgustavo has done it, but what I've done so far myself is implement a simple Middleware for Gin which is added to the handler chain if a NEWRELIC_KEY environment variable is set.

This middleware for each request creates the NR transaction, gives it a default name and properties and then puts the transaction on the gin context so the request handler can annotate it further.

A simplest middleware would look like this:

package middleware

import newrelic "github.com/newrelic/go-agent"

func NewRelic(license string) gin.HandlerFunc {
    app := newrelic.NewApplication("My App Name", license)
    return func(c *gin.Context) {
        tx := app.StartTransaction(c.Request.URL.Path, c.Writer, c.Request)
        c.Set("newrelic.tx", tx)
        c.Next()
    }
}

Then in your handler you can call tx := c.MustGet("newrelic.tx").(newrelic.Transaction) to get the transaction.

@rodriguezgustavo
Copy link

Hi @LeoAdamek,

Yo can use nrgin.Middleware(newrelicApp) to avoid writting your custom middleware. Then in your gingonic handlers you can access to the transaccion calling to nrgin.Transaction(*gin.Context)

@willnewrelic
Copy link
Author

Hi All

This work has been merged into master and is now released.

@sudo-suhas
Copy link

@willnewrelic Not sure if I should open a new issue but status codes are not captured correctly in newrelic.

For this sample code:

ctx.JSON(
	http.StatusUnprocessableEntity, // 422
	util.ErrorResp(validationErrMsg, vErrs),
)

... newrelic shows the httpResponseCode as 200:

image

And a header which was sent in the request is missing too.

@AndyEsser
Copy link

Does New Relic correctly capture the HTTP Status Code now of requests? I have data populating in my dashboard, but when a response is 400 or 403 I don't see any errors displayed

@crenshaw
Copy link

Thanks @AndyEsser for your question. I will submit a ticket for this question and get back to you.

@periclesroo
Copy link

Are there any updates on what @sudo-suhas reported? I have pretty much the same issue, we raise a 500, the response code is 500 but new relic reports a 200.

@sudo-suhas
Copy link

So here's the work around I used:

  • Turn off tracking for the response code using the config passed to newrelic.NewApplication.
  • Add a middleware to track the response code manually.
cfg := newrelic.NewConfig("myapp-"+config.App.Env, config.App.NewrelicLicense)

// With gin, newrelic is not able to capture HTTP response code correctly.
// So we disable the defaults as shown here -
// https://github.com/newrelic/go-agent/blob/master/GUIDE.md#attributes
// and track it ourself.
cfg.Attributes.Exclude = append(cfg.Attributes.Exclude, newrelic.AttributeResponseCode)

var err error
if Agent, err = newrelic.NewApplication(cfg); err != nil {
    log.WithError(err).Panic("Newrelic agent spawn failed!")
}

// trackInNR tracks additional attributes for all requests including errors, if
// any. It only tracks the HTTP response status code as newrelic seems unable to
// track it correctly.
func trackInNR(ctx *gin.Context) {
	ctx.Next()

	if txn := nrgin.Transaction(ctx); txn != nil {
		err := txn.AddAttribute(newrelic.AttributeResponseCode, ctx.Writer.Status())
		if err != nil {
			log.WithError(err).Warn("Got error while trying to track attribute")
		}

		errMsgs := ctx.Errors.ByType(gin.ErrorTypePublic | gin.ErrorTypePrivate)
		if len(errMsgs) > 0 {
			qryStr := ctx.Request.URL.RawQuery
			txn.AddAttribute("queryParams", qryStr)

			for _, errMsg := range errMsgs {
				if err := txn.NoticeError(errMsg.Err); err != nil {
					log.WithError(err).Warn("Got error while trying to track attribute")
				}
				// For now, not trying to send the meta to newrelic.
				// `txn.AddAttribute` will only accept and send string, numbers
				// and booleans. We'd need to flatten all the values recursively
				// which might be costly.
			}
		}
	}
}

// and use middleware
router.Use(nrgin.Middleware(monitor.Agent), trackInNR)

@willnewrelic
Copy link
Author

willnewrelic commented Oct 24, 2018

Hi all

Regarding the http response codes: Yes, unfortunately using any gin.Context methods that delegate to Context.Render (such as Context.JSON) will result in missing response codes. This is a consequence of the Context.Status method not using the Writer field which our instrumentation has replaced.

I have created a PR against Gin to get this fixed here: gin-gonic/gin#1606

Hoping that gets some love soon!

@willnewrelic willnewrelic deleted the PR-gin branch December 19, 2018 02:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants