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.

Sign up for GitHub

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 sygnal_inflight_request_limit_drop #146

Merged
merged 1 commit into from
Aug 6, 2020
Merged

Add Prometheus metric for sygnal_inflight_request_limit_drop #146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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/146.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add a Prometheus metric (`sygnal_inflight_request_limit_drop`) that shows the number of notifications dropped due to exceeding the in-flight concurrent request limit.
12 changes: 12 additions & 0 deletions sygnal/notifications.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
import typing
from typing import Any, Dict, List, Optional

from prometheus_client import Counter

from .exceptions import InvalidNotificationException, NotificationDispatchException

if typing.TYPE_CHECKING:
Expand Down Expand Up @@ -142,6 +144,13 @@ class ConcurrencyLimitedPushkin(Pushkin):

UNDERSTOOD_CONFIG_FIELDS = {"inflight_request_limit"}

RATELIMITING_DROPPED_REQUESTS = Counter(
Copy link
Member

Choose a reason for hiding this comment

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

question: if there are two pushkins (say, for GCM and APNS), do they both correctly refer to the same instance of Counter ? or do they each get their own instance (which will probably confuse the prometheus lib)?

(I can never quite remember how class-level attributes work)

Copy link
Member

Choose a reason for hiding this comment

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

They should get the same one. I wasn't 100% sure that's what was wanted, but I think it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding was/is that the class attribute is initialised when the class declaration is executed, and that attribute stays with the class.

Note it is always referred to as ConcurrencyLimitedPushkin.RATELIMITING_DROPPED_REQUESTS, so it would always act like a namespaced global.

I didn't know whether subclasses would get access to it (not that it matters, since we access the field statically every time and not through self or cls), so I tried it:

oli@bbm-neon:~$ python3
Python 3.6.9 (default, Jul 17 2020, 12:50:27) 
[GCC 8.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> class X:
...     bob = []
... 
>>> class Y(X):
...     pass
... 
>>> class Z(X):
...     pass
... 
>>> id(Y.bob)
140481390301896
>>> id(X.bob)
140481390301896
>>> id(Z.bob)
140481390301896

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(and to answer the question, yes this is intended — the pushkin is distinguished by the pushkin label, as this is what Prometheus understands)

Copy link
Member

Choose a reason for hiding this comment

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

In your example, even doing the following should return the same thing:

id(Y().bob)

Doesn't matter if you access it via the class name or not (although I think it is clearer to access class-level variables that way).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In your example, even doing the following should return the same thing:

id(Y().bob)

Doesn't matter if you access it via the class name or not (although I think it is clearer to access class-level variables that way).

Yes you are right (and I suspected so), but my experience from Java is that 'access static vars by class name, even if you can do it by instance' :)

"sygnal_inflight_request_limit_drop",
"Number of notifications dropped because the number of inflight requests"
" exceeded the configured inflight_request_limit.",
labelnames=["pushkin"],
)

def __init__(self, name: str, sygnal: "Sygnal", config: Dict[str, Any]):
super(ConcurrencyLimitedPushkin, self).__init__(name, sygnal, config)
self._concurrent_limit = config.get(
Expand All @@ -154,6 +163,9 @@ async def dispatch_notification(
self, n: Notification, device: Device, context: "NotificationContext"
) -> List[str]:
if self._concurrent_now >= self._concurrent_limit:
ConcurrencyLimitedPushkin.RATELIMITING_DROPPED_REQUESTS.labels(
pushkin=self.name
).inc()
raise NotificationDispatchException(
"Too many in-flight requests for this pushkin. "
"(Something is wrong and Sygnal is struggling to keep up!)"
Expand Down