fix(metrics): use rule as path #150
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
Conversation
|
Interesting! good change! :) |
Using the real path as metric created useless metrics. When using a dynamic rule, it creates unique paths with every metric, which makes it impossible to aggregate metrics to the same endpoint.
48fd194 to
cc62604
Compare
| @@ -30,9 +30,14 @@ def before_request(self): # pylint: disable=R0201 | |||
| request.start_time = time.time() | |||
|
|
|||
| def after_request(self, response): | |||
| if hasattr(request.url_rule, "rule"): | |||
There was a problem hiding this comment.
@avara1986 Do you know if there's a real possibility that a request wouldn't have this attribute? I've tested locally and even without using dynamic values for the endpoints works. But the test doesn't, without the conditional. I'm not sure if it really should be like this and have a fallback or is the test_client that's not really complete.
There was a problem hiding this comment.
@alexppg the problem is not in tests. request.url_rule is None when you get 404 response. It makes sense if you visit a non-exists page therefore, you haven't a url_rule.
PR approved
PD: sorry, one week late... 🙈
Pull Request Test Coverage Report for Build 876
💛 - Coveralls |
|
Shouldn't the CI execute the tests? @avara1986 |
Yess, but sometimes Travis has a big queue of jobs 😛 |
Using the real path as metric created useless metrics. When using a
dynamic rule, it creates unique paths with every metric, which makes it
impossible to aggregate metrics to the same endpoint.
For example, having the next endpoint:
Created the next metrics:
With those metrics, it's impossible to filter that method, since the
uriis always different. With this fix, the generated metrics are: