8000 add analytics for SNS internal routes by bentsku · Pull Request #11862 · localstack/localstack · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 1 commit into from
Nov 20, 2024
Merged

add analytics for SNS internal routes #11862

merged 1 commit into from
Nov 20, 2024

Conversation

bentsku
Copy link
Contributor
@bentsku bentsku commented Nov 18, 2024

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

  • implement analytics for SNS internal routes via a base class + decorator on routes

@bentsku bentsku added aws:sns Amazon Simple Notification Service semver: patch Non-breaking changes which can be included in patch releases labels Nov 18, 2024
@bentsku bentsku self-assigned this Nov 18, 2024
@bentsku bentsku added this to the 4.0 milestone Nov 18, 2024
@giograno
Copy link
Member

FYI, we might have some overlap with #11855

@bentsku
Copy link
Contributor Author
bentsku commented Nov 18, 2024

@giograno yes, I think there might be some issue with #11855 because it will start counting every path for every internal route, even API Gateway which can technically have a lot of different path. I don't know how we can make it opt-in / opt-out, and maybe more granular?

Copy link

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   1h 5m 19s ⏱️ - 38m 42s
2 129 tests  - 1 397  1 948 ✅  - 1 185  181 💤  - 212  0 ❌ ±0 
2 131 runs   - 1 397  1 948 ✅  - 1 185  183 💤  - 212  0 ❌ ±0 

Results for commit 5c26b71. ± Comparison against base commit 5b44747.

This pull request removes 1397 tests.
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_func
8000
tion_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

@bentsku bentsku requested a review from thrau November 18, 2024 21:06
@bentsku bentsku marked this pull request as ready for review November 18, 2024 21:06
@bentsku bentsku requested a review from baermat as a code owner November 18, 2024 21:06
Copy link
Member
@thrau thrau left a 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!

Comment on lines +1131 to +1133
def _wrapper(self, *args, **kwargs):
self.count_usage()
return f(self, *args, **kwargs)
Copy link
Member

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?

Suggested change
def _wrapper(self, *args, **kwargs):
self.count_usage()
return f(self, *args, **kwargs)
def _wrapper(*args, **kwargs):
self.count_usage()
return f(*args, **kwargs)

Copy link
Contributor Author
@bentsku bentsku Nov 20, 2024

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 👀

Copy link
Contributor Author

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?

Copy link
Member

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!

Copy link
Contributor Author
@bentsku bentsku Nov 20, 2024

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!

@thrau thrau merged commit 642e645 into master Nov 20, 2024
38 checks passed
@thrau thrau deleted the add-analytics-sns branch November 20, 2024 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws:sns Amazon Simple Notification Service semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0