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 prometheus metric for the number of requests in flight. #87

Merged
merged 5 commits into from
Mar 24, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/87.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add prometheus metric for the number of requests in flight.
23 changes: 16 additions & 7 deletions sygnal/http.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,20 +21,21 @@
import traceback
from uuid import uuid4

from opentracing import Format, tags, logs
from prometheus_client import Counter, Histogram
from opentracing import Format, logs, tags
from prometheus_client import Counter, Gauge, Histogram
from twisted.internet.defer import ensureDeferred
from twisted.web import server
from twisted.web.http import (
proxiedLogFormatter,
datetimeToLogString,
combinedLogFormatter,
datetimeToLogString,
proxiedLogFormatter,
)
from twisted.web.resource import Resource
from twisted.web.server import NOT_DONE_YET

from sygnal.notifications import NotificationContext
from sygnal.utils import NotificationLoggerAdapter

from .exceptions import InvalidNotificationException, NotificationDispatchException
from .notifications import Notification

Expand Down Expand Up @@ -66,6 +67,12 @@
labelnames=["code"],
)

REQUESTS_IN_FLIGHT_GUAGE = Gauge(
"sygnal_requests_in_flight",
"Number of HTTP requests in flight",
labelnames=["resource"],
)


class V1NotifyHandler(Resource):
def __init__(self, sygnal):
Expand Down Expand Up @@ -164,9 +171,11 @@ def _handle_request(self, request):

root_span_accounted_for = True

ensureDeferred(
self._handle_dispatch(root_span, request, log, notif, context)
)
async def cb():
with REQUESTS_IN_FLIGHT_GUAGE.track_inprogress():
await self._handle_dispatch(root_span, request, log, notif, context)
Copy link
Member

Choose a reason for hiding this comment

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

Can we not just instead do:

with REQUESTS_IN_FLIGHT_GUAGE.track_inprogress():
    await self._handle_dispatch(root_span, request, log, notif, context)

rather than having the request itself track what's going on? Or can we finish the request independently of the _handle_dispatch?

Copy link
Member Author

Choose a reason for hiding this comment

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

well, the intention was to lay the groundwork for tracking requests in progress when we have other servlets which don't necessarily follow the same pattern. In principle, yes we can finish a request independently of the async handler - for example if we hand over control to a Producer to stream back a response.

In practice, we dont' do that anywhere in sygnal and honestly I guess it's unlikely that we ever will. So yeah, this is over-engineered. I'll simplify it.


ensureDeferred(cb())

# we have to try and send the notifications first,
# so we can find out which ones to reject
Expand Down