New Default Metric: Requests In Progress

[scotgopal]

Addressed issue #26 in this PR. Along with that, I've made some additional changes too.

1. Added pytest dependency into requirements.txt.
2. Minor correction in README.md.

This is my very first contribution to an external open source project. Feel free to comment on the contribution and I'll be glad to learn and improve. Thank you.

[stephenhillier]

Hi @scotgopal I think this is a great idea, thanks for contributing this.

I made some suggestion comments to update the new metric name to requests_in_progress.

It'd be nice to get at least one unit test for this as well. If you are able to do that then great, if not, I can probably add one on the weekend.

[scotgopal]

Hi @scotgopal I think this is a great idea, thanks for contributing this.

I made some suggestion comments to update the new metric name to requests_in_progress.

It'd be nice to get at least one unit test for this as well. If you are able to do that then great, if not, I can probably add one on the weekend.

Thanks a lot for the suggestions and comments @stephenhillier . I'd work on creating a unit test tomorrow and will do a commit to this branch.

[scotgopal]

Hello @stephenhillier . I realized that there had been a mistake during the last test. I've installed the package from pip instead of installing it from the dev folder using pip install -e ., meaning the changes were not even tested. Upon reinstalling, the new default metric fails 3 of the existing 21 tests.

Guess I will have to fix that before making new tests.

Talking about the test for the new default metrics: requests_in_progress, I had an idea of testing it by:

1. Instantiating three TestClients with default prefix
2. Two clients sends a GET request to a new endpoint /sleep, which would just make the two threads sleep for 5 seconds.
3. The third client sends a GET /metrics.
4. Assert if starlette_requests_in_progress{app_name="starlette", method="GET", path="/200"} 2.0 exists in the third client's response.

What do you think about this idea? Also could you guide on how to simulate concurrent requests?

[stephenhillier]

That's worth a try. I previously used sleep(0.1) for another test; that should be plenty long enough to keep the gauge incremented.

As soon as I get a chance to sit down tonight or tomorrow I'll try out a couple ways of triggering the gauge and let you know.

[stephenhillier]

Ah ok, creating the path label here https://github.com/stephenhillier/starlette_exporter/pull/27/files#diff-20b93ddcf1235118346c28f13a10a6057c7772939f4a72cfbafc62d998d5aa3bR108 is what's causing the tests to fail.

There are a couple tests that ensure that the path label is not populated with paths that don't exist (to prevent unbounded label cardinality), but we don't know that until the end of the request (at least not the way it's currently set up).

What do you think about excluding the path label from the requests_in_progress gauge? Would it be less useful to you?

[stephenhillier]

There's a really basic example of a test for this PR in this commit (on a separate branch): ca5e46f

The test could be improved, however, I think this at least tests that the gauge was incremented and then decremented.

[scotgopal]

Ah ok, creating the path label here https://github.com/stephenhillier/starlette_exporter/pull/27/files#diff-20b93ddcf1235118346c28f13a10a6057c7772939f4a72cfbafc62d998d5aa3bR108 is what's causing the tests to fail.

There are a couple tests that ensure that the path label is not populated with paths that don't exist (to prevent unbounded label cardinality), but we don't know that until the end of the request (at least not the way it's currently set up).

What do you think about excluding the path label from the requests_in_progress gauge? Would it be less useful to you?

Hey there. I think excluding the path label will increment the gauge for calls to every endpoint in the server. We can opt to do it this way as a temporary fix and to make this default metric available to the users. This info will still be useful to people.

But what I had in mind when I came up with the idea was that by using the path label, users will have the capability to know which endpoint is the most active at a given time. Also, different endpoints may process input at different speeds, so without being able to filter by path, we wouldn't know which endpoint is still processing and which endpoint is done.

[stephenhillier]

Ok, that sounds good.
I have some ideas for being able to include the path, but I'll have to do more testing. I might merge this in without the path label and work on that next. PS, this feature comes at a good time; I want to add this metric to a project so thanks again for contributing it. You're right that knowing the path would be really useful.

[scotgopal]

Ok, that sounds good.
I have some ideas for being able to include the path, but I'll have to do more testing. I might merge this in without the path label and work on that next. PS, this feature comes at a good time; I want to add this metric to a project so thanks again for contributing it. You're right that knowing the path would be really useful.

Wonderful. Glad to know we're on the same page for this feature. I am also feeling lucky to have my first contribution to be quite useful. 😇 Thanks for this opportunity. Let's merge this in for now.💪

[stephenhillier]

Do you want to git cherry-pick ca5e46f05cab9ae7ae49b7b654d2cf85e9928c69 (my commit from my comment above), or merge my inprogress branch into yours? That way we can get the tests passing

[scotgopal]

To be honest, I never heard of git cherry-pick before. 😅 I'd like to give that a try.

[scotgopal]

Do let me know if I missed anything out @stephenhillier .