-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
add analytics for SNS internal routes #11862
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
Conversation
FYI, we might have some overlap with #11855 |
LocalStack Community integration with Pro 2 files ± 0 2 suites ±0 1h 5m 19s ⏱️ - 38m 42s Results for commit 5c26b71. ± Comparison against base commit 5b44747. This pull request removes 1397 tests.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! one minor nit
i think it would be great to solve this more generically across endpoints moving forward, but good for now to get some initial readings!
def _wrapper(self, *args, **kwargs): | ||
self.count_usage() | ||
return f(self, *args, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps we don't need self
?
def _wrapper(self, *args, **kwargs): | |
self.count_usage() | |
return f(self, *args, **kwargs) | |
def _wrapper(*args, **kwargs): | |
self.count_usage() | |
return f(*args, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will complain, we do need self
passed, it failed before I added it 😄
edit: oops, misread the suggested change. let me check 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I don't pass self, then I need to extract it from args
right, otherwise self.count_usage()
will raise an exception as it wouldn't be in scope?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh ,whoops, didn't see that there, of course your right!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not too fond of this either tbh, for a one line change I could just call self.count_usage()
inside the method itself... it's the same as decorating it 😬 it just feels wrong to add unrelated code inside the method logic
I could also maybe auto-wraps methods starting with on_
in a similar way to the resource thing. Anyway... this will do until we rework it!
Motivation
As part of our effort to improve our analytics, this PR implements usage counters for SNS internal routes.
We can now properly have usage data on SNS internal routes invocations, to understand their usage better.
Usage is separated on what the internal route is targeting, be it Platform Endpoint messages, SMS messages or Subscription Tokens (useful for example for email subscriptions).
Changes