Skip to content

add analytics to APIGW NextGen REST API handler chain #11860

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

Jump to bottom
Merged
merged 1 commit into from
Nov 19, 2024
Merged

add analytics to APIGW NextGen REST API handler chain #11860

merged 1 commit into from
Nov 19, 2024

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Nov 18, 2024 •
edited
Loading

Motivation

As part of our effort to improve our analytics, this PR implements usage counters for API Gateway.

We can now properly have usage data on APIGW invocations, with the integration type (AWS_PROXY, MOCK etc), and in case of the AWS integration, the service the integration is targeting (dynamodb, sqs etc..)

Changes

  • implement APIGW NextGen handler for analytics

@bentsku bentsku added aws:apigateway Amazon API Gateway 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 changed the title add analytics to APIGW NextGen handler chain add analytics to APIGW NextGen REST API handler chain Nov 18, 2024
@github-actions GitHub Actions
Copy link

LocalStack Community integration with Pro

  2 files  ±    0    2 suites  ±0   26m 27s ⏱️ - 1h 17m 34s
808 tests  - 2 718  766 ✅  - 2 367  42 💤  - 351  0 ❌ ±0 
810 runs   - 2 718  766 ✅  - 2 367  44 💤  - 351  0 ❌ ±0 

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

This pull request removes 2718 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_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

@bentsku bentsku added this to the 4.0 milestone Nov 18, 2024
@bentsku bentsku mentioned this pull request Nov 18, 2024
Open
@bentsku bentsku requested a review from thrau November 18, 2024 21:02
@bentsku bentsku marked this pull request as ready for review November 18, 2024 21:02
@bentsku bentsku requested a review from cloutierMat as a code owner November 18, 2024 21:02
cloutierMat
cloutierMat approved these changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@cloutierMat cloutierMat left a comment

Choose a reason for hiding this comment

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

Thanks for adding this! So awesome how clean it is to add this as a handler! 🚀

I have a couple questions as to the relevancy of some of the captured data point. If you feel those are important, let's merge. But if not, we might be better to eliminate some noise? 🤔

localstack-core/localstack/services/apigateway/next_gen/execute_api/handlers/analytics.py
Comment on lines +30 to +32
# if the invocation does not have an integration attached, it probably failed before routing the request,
# hence we should count it as a NOT_FOUND invocation
invocation_type = "NOT_FOUND"
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: I wonder if this is a relevant data point? On it's own, what intel can we gather from it, except for maybe knowing the total amount of requests sent to apigw? But is it relevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be good to know the total amount of requests sent to the endpoint. Not very relevant directly, but I think relevant if we do the sum of all types regardless of their value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe @thrau could chime in too, I also see the point, so yeah I think it depends on expectations!

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, I am OK either way! 👍 I was simply raising a question, to make sure we are deliberate with logging 😉

localstack-core/localstack/services/apigateway/next_gen/execute_api/handlers/analytics.py
Comment on lines +38 to +42
if not integration_uri:
return "null"

if len(split_arn := integration_uri.split(":", maxsplit=5)) < 4:
return "null"
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Same here. It seems we are we just capturing misconfigured Api? Is it relevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The check is mostly to avoid exceptions, making sure we have a proper value to send

@bentsku bentsku merged commit d77e4d3 into master Nov 19, 2024
40 checks passed
@bentsku bentsku deleted the add-analytics-apigw branch November 19, 2024 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cloutierMat cloutierMat cloutierMat approved these changes

@thrau thrau Awaiting requested review from thrau

Assignees

@bentsku bentsku

Labels
aws:apigateway Amazon API Gateway semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
2 participants
@bentsku @cloutierMat