Add optional func to set the span's error tag flag

[jmdalmeida]

Some APM providers (like Datadog) require the span error tag to be set in order to properly display the status of the trace.
This PR will add a new MWOption function that allows to set the flag based on gin's context.

[stuart-mclaren]

This isn't present in the library that this one was derived from: https://github.com/opentracing-contrib/go-stdlib. Ideally I'd like not to diverge from that too much.

Can you give an example of what the use case is?

If the error tag was set by the underlying opentracing library, is it the case that this patch would over write the value to 'false' by default?

[jmdalmeida]

Hey @stuart-mclaren,

My usecase is the following:

opNameFn := ginhttp.OperationNameFunc(func(r *http.Request) string {
	return fmt.Sprintf("%s:%s", strings.ToLower(r.Method), r.RequestURI)
})
errorFn := ginhttp.MWErrorFunc(func(ctx *gin.Context) bool {
	return ctx.Writer.Status() >= 500
})

return ginhttp.Middleware(opentracing.GlobalTracer(), opNameFn, errorFn)

Basically, every time I'm returning status >= 500, it'll set the error tag to true.

If the func is not set, this will overwrite the error tag of the top-most span of the request but not child spans, yes. Hadn't thought of that possibility. Unfortunately, the span interface doesn't expose the tags that have been set or else I could make the current error tag the default. Not sure how to avoid this.

[stuart-mclaren]

You could register a second very small (~3 line) custom middleware -- would that work for you (and avoid the writing over problem -- in this library at least)?

[jmdalmeida]

How about this? Basically it doesn't set a default and only calls the error func if it was explicitly set on the project.

[stuart-mclaren]

How about this?

It's less an error function and more like the existing

opts.spanObserver(sp, c.Request)

except it's after the call. It also has the full gin context (which doesn't leave much constraint on what people can do). And since it doesn't require the span its signature is pretty much the same as a regular gin middleware.

You could consider proposing something to https://github.com/opentracing-contrib/go-stdlib to see what they say. (Eg would they accept an errorFunc that just took an int as its argument?)For reference opentracing/specification#96

(Funnily enough I've a query to them at the moment too opentracing-contrib/go-stdlib#33)

[jmdalmeida]

I see your point. Would you suggest having a more generic after-request function similar to spanObserver then?
You also suggested implementing a separate middleware. It would have to be a post-request middleware that runs before the sp.Finish() call, right?

It also has the full gin context (which doesn't leave much constraint on what people can do).

Eg would they accept an errorFunc that just took an int as its argument?

Actually I ended up changing my implementation to

errorFn := ginhttp.MWErrorFunc(func(ctx *gin.Context) bool {
	return ctx.Writer.Status() >= 400 || len(ctx.Errors) > 0
})

so I think there's a clear benefit to having the full gin.Context available. Not sure how this could be applied to the https://github.com/opentracing-contrib/go-stdlib implementation. Maybe also include context.Context?

nethttp.MWErrorFunc(f func(ctx context.Context, statusCode int) bool) MWOption 

[stuart-mclaren]

Hi,

I see that opentracing-contrib/go-stdlib are now doing this:

		if sct.status >= http.StatusInternalServerError {
			ext.Error.Set(sp, true)
		}

that seems to match your use case:

return ctx.Writer.Status() >= 500

would backporting the new opentracing-contrib/go-stdlib behaviour work for you?

[jvexiau]

hi @stuart-mclaren ,

I think just backporting will do the job ! do you want a PR ?
just need to add this before sp.Finish() :

if c.Writer.Status() >= http.StatusInternalServerError {
	ext.Error.Set(sp, true)
}

thanks !