go-http-metrics icon indicating copy to clipboard operation
go-http-metrics copied to clipboard

Reduce HandlerID cardinality by default in gin middleware

Open arwineap opened this issue 2 years ago • 4 comments

Here's the doc for FullPath()

FullPath returns a matched route full path. For not found routes
returns an empty string.
    router.GET("/user/:id", func(c *gin.Context) {
        c.FullPath() == "/user/:id" // true
    })

IMO it's better to do the correct thing by default when possible; existing users would benefit from performance increases but it may break some dashboards.

Let me know what you think, Thanks!

arwineap avatar Nov 18 '21 04:11 arwineap

Hi @arwineap!

Thanks for the contribution, You are right, safety should be a default. However, this will have unexpected behaviour for users at this moment, and that could be worse. Maybe would be better to have a flag on middleware creation and break the API so when people upgrade, they are aware of this safer behavior change that will be by default. e.g:

func Handler(handlerID string, m middleware.Middleware, rawPaths bool) gin.HandlerFunc

Let me think about it!

slok avatar Dec 03 '21 07:12 slok

Just a bump on this as I found myself wondering why there wasn't an option for this, so I ended up just implementing it myself locally.

+1 for FullPath()

ImVexed avatar Jan 23 '22 09:01 ImVexed

@arwineap @ImVexed Ok, let's do this, add an option on middleware creation so it breaks the API and users are aware of this change. Similar to what is above, by default would use FullPath and if the flag of RawPaths is used then, we use the current way of measuring it.

WDYT?

slok avatar Feb 11 '22 16:02 slok

I'm doing something similar for gorestful in #150. Instead of adding a rawPaths bool to Handler, why not consider adding a Config object specific for gin so that we can easily add more options later if that ends up being important.

In #150, I also introduced HandlerWithConfig and left Handler alone (Handler would then be backward compatible and just call HandlerWithConfig with an empty Config{}).

tekkamanendless avatar Jul 21 '22 14:07 tekkamanendless