Skip to content
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

Add metric for currently-in-flight HTTP requests #1378

Merged
merged 10 commits into from
Mar 16, 2021

Conversation

evankanderson
Copy link
Contributor

@evankanderson evankanderson commented Jan 26, 2021

Changes

Adds an http.server.requests metric to count in-flight HTTP requests on the server.

I was starting to implement some monitoring using the HTTP semantic conventions, and realized that it was also useful to have a count of the current in-flight HTTP requests (those which had started but not yet completed). This allows monitoring both the concurrency of the server and detecting unfortunate queueing or backlog events before the requests have completed (for example, if the request is blocked indefinitely).

(Will update CHANGELOG.md once I have a PR #)

@evankanderson evankanderson requested a review from a team as a code owner January 26, 2021 17:57
@evankanderson evankanderson requested a review from a team January 26, 2021 17:57
@evankanderson evankanderson requested a review from a team as a code owner January 26, 2021 17:57
Base automatically changed from master to main January 27, 2021 21:16
@github-actions
Copy link

github-actions bot commented Feb 4, 2021

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Feb 4, 2021
@evankanderson
Copy link
Contributor Author

(Merged CHANGELOG.md -- let me know if I should squash)

@github-actions github-actions bot removed the Stale label Feb 8, 2021
@evankanderson
Copy link
Contributor Author

Is this a reasonable addition? Do I need to bring this up at a contributor meeting?

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Thanks!

reyang
reyang previously requested changes Feb 18, 2021
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

Please see my comment here.

@jmacd
Copy link
Contributor

jmacd commented Feb 24, 2021

@reyang In other cases, we've discussed how it may be acceptable to specify semantic conventions without saying how they should be generated. For the case of "in flight HTTP requests" we could either materialize a dedicated metric or we could calculate the number in-flight by subtracting one metric from another. It's clearly much harder to do the second, so most users will prefer the first. But if you have a sophisticated metric system, it's redundant to record an additional metric and you shouldn't have to.

The example we discussed was metrics with .utilization, .usage, and .limit suffixes. With any two of these you can calculate the third, so which should we record? All three are useful to monitor, so it's hard to argue that in some cases you shouldn't just record all three.

@reyang reyang dismissed their stale review February 25, 2021 02:20

I agree with what @jmacd explained here #1378 (comment)

Co-authored-by: Reiley Yang <reyang@microsoft.com>
@evankanderson
Copy link
Contributor Author

Sorry for the delay, the extra comments caught me in the middle of vacation/school break, and it took some time to dig out.

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Mar 10, 2021
@evankanderson
Copy link
Contributor Author

I'm not authorized to merge this PR, but it looks like it does have the requisite reviews. What's the next step?

@evankanderson
Copy link
Contributor Author

@jmacd -- I think this is ready for merge?

@github-actions github-actions bot removed the Stale label Mar 13, 2021
@jmacd
Copy link
Contributor

jmacd commented Mar 16, 2021

@evankanderson sorry for the long delay. Thank you for this contribution!

@jmacd jmacd merged commit 37d7334 into open-telemetry:main Mar 16, 2021
ThomsonTan pushed a commit to ThomsonTan/opentelemetry-specification that referenced this pull request Mar 30, 2021
* Add metric for currently-in-flight HTTP requests

* Update name to active_requests

* Fix missing rename noticed by anuraaga

* Clarify `active_requests` are in-flight

Co-authored-by: Reiley Yang <reyang@microsoft.com>

* Switch to UpDownSumObserver

Co-authored-by: Reiley Yang <reyang@microsoft.com>
Co-authored-by: Joshua MacDonald <jmacd@users.noreply.github.com>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants