Conversation
jcgruenhage
left a comment
jcgruenhage
left a comment
There was a problem hiding this comment.
Some comments
| let mut path = String::from(path); | ||
| for (exp, repl) in self.replacements.clone() { | ||
| path = String::from(exp.replace_all(&path, &repl as &str)) | ||
| } |
There was a problem hiding this comment.
I'm not really happy with this as it is, because each time we're updating metrics, we copy all replacements, the path once, and then the path again for each replacement. I couldn't find a way to do this with less copies though that also makes the borrow checker happy.
|
|
||
| Ok((http_requests_total, http_requests_duration_seconds)) | ||
| } | ||
|
|
There was a problem hiding this comment.
Ideally a cfg attribute on the parameter would make duplicating this method unnecessary, but that would require rust nightly, since attributes on parameters are not stabilized yet (proposed to be stabilized soon though!)
There was a problem hiding this comment.
I believe it's now in 1.39 so this can be changed.
There was a problem hiding this comment.
It requires that we bump the minimum rust version to 1.39, if that's okay we can do that.
| futures = "0.1" | ||
| prometheus = "0.7" | ||
| regex = { version = "1.3.1", optional = true } | ||
| log = "0.4.8" |
There was a problem hiding this comment.
There should be more logging in this crate IMO, added the log crate to enable logging.
jcgruenhage
force-pushed
the
regex-replace
branch
from
b4077b3 to
7fdccd2
Compare
|
force push to rebase onto the new changes on master |
|
@nlopes this is marked as WIP because there is a bug in here somewhere. In the app I wrote this for, once switch from master to this version, for some endpoints metrics aren't recorded, and sometimes already recorded metrics also get lost and it all starts over, without any panics or errors in the logs. With the version from the master branch, it all works fine. Could you have a look? |
jcgruenhage
force-pushed
the
regex-replace
branch
from
7fdccd2 to
b37fa4b
Compare
nlopes
left a comment
nlopes
left a comment
There was a problem hiding this comment.
I think this needs a few tests overall. I'm also not yet 100% on this but I'm warming up to the idea.
On your concern about the bug, I personally couldn't find it through my experimentation with your branch - maybe tests will help?
Thanks for submitting this, I really appreciate it.
|
|
||
| Ok((http_requests_total, http_requests_duration_seconds)) | ||
| } | ||
|
|
There was a problem hiding this comment.
I believe it's now in 1.39 so this can be changed.
|
I'm currently occupied with deployments. Once I'm back from in dev land from my ops tasks, I'll update this with 1.39 in mind and try to reproduce the bug I was experiencing. |
|
I believe #20 will make this mostly unnecessary |
3 participants
This allows users to specify a set of regexes that should be replaced in the path, to avoid having a gigantic set of metrics when the path contains resource IDs