-
-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: configurable path parameters cardinality #73
Conversation
@nlopes You may take a look at this |
Thanks for sending over. I'll try to take a look sometime this week. |
Hello, I'm kindly asking if you have some update on this, thanks! 😄 |
Apologies for taking so long. Can you modify your example please? As it stands I don't think it actually exemplifies your change. Or am I misunderstanding it? |
Yes you are correct, I will update the example. |
the main purpose for me is to have an example on which I can test with configurable params cardinality feature.
Allow developer to externally set which on route params they want to keep in the metrics (as metrics label cardinality) on a particular route.
In theory I should have rebased this fork, and now I added an example. I also need to fix the code so that 404 are not counting as new labels values. TODO:
|
@nlopes can you take a look at this? It should be better now |
Hello @nlopes sorry to ask you again, what can be done to get this forward? I know that maintaining opensource software is a burden 😢 . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there. Apologies, I've forgotten about this.
README.md
Outdated
### Configurable routes pattern cardinality | ||
|
||
Let's say you have on your app a route to fetch posts by language and by slug `GET /posts/{language}/{slug}`. | ||
By default, actix-web-prom will provide metrics for the whole route with the label `endpoint` set to the pattern `/posts/{locale}/{slug}`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, actix-web-prom will provide metrics for the whole route with the label `endpoint` set to the pattern `/posts/{locale}/{slug}`. | |
By default, actix-web-prom will provide metrics for the whole route with the label `endpoint` set to the pattern `/posts/{language}/{slug}`. |
src/lib.rs
Outdated
|| self.exclude_status.contains(&status) | ||
{ | ||
return; | ||
} | ||
|
||
let final_pattern = | ||
if fallback_pattern != pattern && (status == 404 || status == 405) { fallback_pattern } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment here on why this is necessary? I'm pretty sure we'll all forget why at some point.
src/lib.rs
Outdated
match strfmt(&full_pattern, ¶ms) { | ||
Ok(mixed_cardinality_pattern) => mixed_cardinality_pattern, | ||
Err(_) => { | ||
// may be we need to log something to warn that we didn't managed to build the pattern? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, probably useful to bring in the log
package.
fix: add warn log when failed to build mixed cardinality pattern docs: typo fix (apply review suggestions)
Thanks for your review, I've fix something |
the idea for this MR was originally discussed in #72
A change that allow developer to change the cardinality of a given param name for a specific route.
To define a config that goes down the line, we need to use actix
extensions_mut()
method. The external/user developer need to define a middleware that will pass some data down the middleware stack.Example when defining a route:
Feel free to comment or help.