Skip to content

fix(metrics): use rule as path #150

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

Merged
merged 2 commits into from
Sep 19, 2020
Merged

fix(metrics): use rule as path #150

merged 2 commits into from
Sep 19, 2020

Conversation

alexppg
Copy link
Member

@alexppg alexppg commented Sep 10, 2020

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:

@self.app.route("/something/<myid>/update")
    def root(myid):

Created the next metrics:

http_server_requests_seconds_count{method="GET",uri="/something/1/update"} 4.0
http_server_requests_seconds_count{method="GET",uri="/something/2/update"} 4.0
http_server_requests_seconds_count{method="GET",uri="/something/3/update"} 4.0
http_server_requests_seconds_count{method="GET",uri="/something/4/update"} 4.0

With those metrics, it's impossible to filter that method, since the uri is always different. With this fix, the generated metrics are:

http_server_requests_seconds_count{method="GET",uri="/something/<myid>/update"} 4.0
http_server_requests_seconds_count{method="GET",uri="/something/<myid>/update"} 4.0
http_server_requests_seconds_count{method="GET",uri="/something/<myid>/update"} 4.0
http_server_requests_seconds_count{method="GET",uri="/something/<myid>/update"} 4.0

@avara1986
Copy link
Member

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.
@alexppg alexppg force-pushed the fix/use_rule_as_path branch from 48fd194 to cc62604 Compare September 10, 2020 18:17
@@ -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"):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figure out this weekend :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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... 🙈

@coveralls
Copy link

coveralls commented Sep 10, 2020

Pull Request Test Coverage Report for Build 876

  • 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 99.25%

Totals Coverage Status
Change from base Build 861: 0.001%
Covered Lines: 1587
Relevant Lines: 1599

💛 - Coveralls

@alexppg alexppg marked this pull request as ready for review September 10, 2020 18:21
@alexppg
Copy link
Member Author

alexppg commented Sep 10, 2020

Shouldn't the CI execute the tests? @avara1986

@avara1986
Copy link
Member

Shouldn't the CI execute the tests? @avara1986

Yess, but sometimes Travis has a big queue of jobs 😛

@avara1986 avara1986 merged commit 03de4f4 into master Sep 19, 2020
@avara1986 avara1986 deleted the fix/use_rule_as_path branch September 19, 2020 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants