fiberprometheus icon indicating copy to clipboard operation
fiberprometheus copied to clipboard

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

Open imrebuild opened this issue 1 year ago • 8 comments

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.

imrebuild avatar Aug 05 '24 20:08 imrebuild

noticed the same issue, this needs to be fixed asap

sashayakovtseva avatar Aug 15 '24 13:08 sashayakovtseva

I will be fixing this during the weekend.

gaby avatar Aug 15 '24 14:08 gaby

@gaby thanks!

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

sashayakovtseva avatar Aug 15 '24 14:08 sashayakovtseva

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.

gaby avatar Aug 16 '24 02:08 gaby

@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

sashayakovtseva avatar Aug 16 '24 14:08 sashayakovtseva

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 avatar Aug 18 '24 13:08 gaby

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 avatar Aug 18 '24 23:08 gaby

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

gaby avatar Aug 19 '24 05:08 gaby

@gaby when can we expect path to be fixed?

0x01F4 avatar Oct 12 '24 11:10 0x01F4

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

  • Added 8-10 more unit tests
  • Fixed the unit buckets
  • Fixed the high cardinality issue
  • Updated dependencies
  • Updated CI / Linter process

gaby avatar Oct 18 '24 13:10 gaby

@gaby Hello, when is the planned merging of this merge request?

buts26 avatar Nov 20 '24 08:11 buts26