search

ansrivas / fiberprometheus

prometheus middleware for Fiber
MIT License
172 stars 37 forks source link

`ctx.Request().RequestURI()` leads to high cardinality metric #209

Open imrebuild opened 3 months ago

imrebuild commented 3 months ago

The value of path changed from ctx.Route().Path to string(ctx.Request().RequestURI()) in commit https://github.com/ansrivas/fiberprometheus/commit/bfc9e019f80791a79e0f780e92d26fa93a3f2d31.

For route /n/:id, it will create metric for each unique id, instead of grouping them under /n/:id.

sashayakovtseva commented 2 months ago

noticed the same issue, this needs to be fixed asap

gaby commented 2 months ago

I will be fixing this during the weekend.

sashayakovtseva commented 2 months ago

@gaby thanks!

additional context: every query parameter is included in metric path now as well

gaby commented 2 months ago

I haven't tested this, but looking at fasthttp docs, I may be able to use string(ctx.Request().URI().Path())

Path returns URI path, i.e. /foo/bar of http://aaa.com/foo/bar?baz=123#qwe .

The returned path is always urldecoded and normalized, i.e. '//f%20obar/baz/../zzz' becomes '/f obar/zzz'.

I have to test what happens if the path has /n/:id.

sashayakovtseva commented 2 months ago

@gaby I tested ctx.Request().URI().Path() with units from this repo and it doesn't help with path parameters, however it strips query params which is good.

I wonder if you might need to use approach similar to this https://github.com/slok/go-http-metrics/blob/master/middleware/middleware.go#L73, where developer can explicitly specify http path for label

gaby commented 2 months ago

I'm working with the Fiber team(I'm one of the Maintainers) to add a way of getting the URL path natively in Fiber. We have the data, it's just not exposed in the public API.

gaby commented 2 months ago

Found a fix, working on documenting the code and sending a PR.

The middleware calls ctx.Next() and the collects metrics, this causes the context values to changed Mid-Middleware execution.

I was able to get the correct full path using ctx.Route().Path.

gaby commented 2 months ago

New problem, can't get the Cache metrics to work at all. It's using similar code to other tests but for some reason /metrics shows as being cached, and not /mypath

0x01F4 commented 3 weeks ago

@gaby when can we expect path to be fixed?

gaby commented 2 weeks ago

@0x01F4 Next week or so. I had to refactor a bunch of stuff. Also removed cache support since it doesnt really work at all.