`ctx.Request().RequestURI()` leads to high cardinality metric
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.
noticed the same issue, this needs to be fixed asap
I will be fixing this during the weekend.
@gaby thanks!
additional context: every query parameter is included in metric path now as well
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 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
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.
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.
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 when can we expect path to be fixed?
@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 Hello, when is the planned merging of this merge request?