fix panic risk in func Middleware

[YueHonghui]

In func Middleware, c.Next would be panic, then span.Finish will not be executed.

//...
	return func(c *gin.Context) {
		carrier := opentracing.HTTPHeadersCarrier(c.Request.Header)
		ctx, _ := tr.Extract(opentracing.HTTPHeaders, carrier)
		op := opts.opNameFunc(c.Request)
		sp := tr.StartSpan(op, ext.RPCServerOption(ctx))
		ext.HTTPMethod.Set(sp, c.Request.Method)
		ext.HTTPUrl.Set(sp, opts.urlTagFunc(c.Request.URL))
		opts.spanObserver(sp, c.Request)

		// set component name, use "net/http" if caller does not specify
		componentName := opts.componentName
		if componentName == "" {
			componentName = defaultComponentName
		}
		ext.Component.Set(sp, componentName)
		c.Request = c.Request.WithContext(
			opentracing.ContextWithSpan(c.Request.Context(), sp))

		c.Next()

		ext.HTTPStatusCode.Set(sp, uint16(c.Writer.Status()))
		sp.Finish()
	}
//...

[stuart-mclaren]

Thanks for your patch YueHonghui.

I'd like to see if we can keep this library and opentracing-contrib/go-stdlib consistent.

[stuart-mclaren]

When there is is a panic "c.Writer.Status" will be equal to 200.

This change means we will successfully complete the trace (sp.Finish()), but the response code will be 200 -- indicating the operation was successful. I wonder if that could be a little misleading?

Looking at https://github.com/opentracing-contrib/go-stdlib (which this library is derived from), our current behaviour matches theirs. When there is a panic the span is not finished (sp.Finish() is not called).

How would you feel about proposing a change similar to this to opentracing-contrib/go-stdlib:

       defer func() {                                                          
            if recover() != nil {                                                                            
                ext.HTTPStatusCode.Set(                                         
                    sp, uint16(http.StatusInternalServerError))                 
            } else {                                                                                            
                ext.HTTPStatusCode.Set(sp, uint16(sct.status))           
            }                                                                                                       
            sp.Finish()                                                         
        }()  

That would mean that they would complete the span with an error if there is a panic. If that (or something similar) was acceptable we could then "backport" the change here.

[stuart-mclaren]

Just noticed https://github.com/opentracing-contrib/go-stdlib !

[stuart-mclaren]

If you reverse this to be
r.Use(mw, gin.Recover())

you can check for a status code of 500 in the panic case and 200 in the non-panic case.

See https://github.com/gin-gonic/gin/blob/master/recovery.go#L68

[YueHonghui]

@stuart-mclaren
It seems much more difficult to fix panic risk with gin comparing to stdlib(opentracing-contrib/go-stdlib#38). As you pointed out, gin's Writer.Status() will be 200 other than 500 when inner handler panicked. This is misleading, and not the same as golang's http implementation. Maybe I need to create an issue to gin's project first.

[polyrabbit]

Dears, any progress on the status code?

Just FYI, my current fix is just to leave the status code as it is:
polyrabbit@e7a5191

[Copilot]

Pull Request Overview

This PR addresses a panic risk in the Middleware function by ensuring that span.Finish is always executed, even if a panic occurs during request processing.

• Reorders defer and c.Next calls in the Middleware function to guarantee span.Finish execution.
• Adds tests in server_test.go to validate span finishing under both normal and panic conditions.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
ginhttp/server_test.go	Adds test cases for normal and panic scenarios to ensure the span finishes.
ginhttp/server.go	Updates the Middleware function to use defer for span.Finish, preventing panic risk.