Events: migrate to new Counter type for Event Rule invocation analytics #12388

bentsku · 2025-03-13T15:28:26Z

Motivation

This PR replaces the analytics Counter used for tracking usage of Event rule invocations. We merge both Counter for success and failure into one, and use the new labels instead.

Changes

merge the 2 counters into one with more labels

Testing

To test the changes:

run a LocalStack in host mode with this branch checked out
run the tests.aws.services.events.test_events_targets.TestEventsTargetSqs.test_put_events_with_target_sqs integration test with TEST_SKIP_LOCALSTACK_START=1 set, to avoid trying to start LocalStack with the test
shutdown LocalStack

I've tried without sending the events but only printing the collected metrics and this is what I got:

{
  "metrics": [
    {
      "namespace": "events",
      "name": "rule_invocations",
      "value": 1,
      "type": "counter",
      "label_1_value": "success",
      "label_2_value": "sqs",
      "label_1": "status",
      "label_2": "service"
    }
  ]
}

github-actions · 2025-03-13T16:15:22Z

LocalStack Community integration with Pro

2 files ± 0 2 suites ±0 1h 10m 3s ⏱️ - 45m 24s
2 521 tests - 1 772 2 416 ✅ - 1 556 105 💤 - 216 0 ❌ ±0
2 523 runs - 1 772 2 416 ✅ - 1 556 107 💤 - 216 0 ❌ ±0

Results for commit 622ad14. ± Comparison against base commit cf90cd4.

This pull request removes 1772 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]
…

♻️ This comment has been updated with latest results.

maxhoheiser

LGTMT - thank you for implementing this :)

maxhoheiser · 2025-03-14T12:18:36Z

localstack-core/localstack/services/events/provider.py

+                        rule_invocation.labels(
+                            status=InvocationStatus.error,
+                            service=target_sender.service,
+                        )


should we maybe also capture the error so we know why the invocation failed - we have to capture something that does not reveals any private information

good point, maybe just the exception type? I'm not sure how many different types we can have, and also in this sense we'd probably need 2 different counters, as the labels would be different for both.

I don't have a lot of context into those and what we actually need for EventBridge, so if you want to add on this PR, you can 😄

vittoriopolverino

LGTM 🥳

vittoriopolverino · 2025-03-14T12:30:53Z

localstack-core/localstack/services/events/analytics.py

+# number of EventBridge rule invocations per target (e.g., aws:lambda)
+# - status label can be `success` or `error`, see InvocationStatus
+# - service label is the target service name
+rule_invocation = Counter(namespace="events", name="rule_invocations", labels=["status", "service"])


question: would it be interesting for you to also include the rule type? (standard, scheduled, etc.) or is that a detail that's not really useful at the moment?

question: is there a risk that the namespace "events" is too generic and could collide with other services? Would it make sense to use something like "eventbridge" instead?

Good point for events, it is the true name of the service so if we were to do a general dashboard based on the list of the official service names, that could be easier, but I can also see the issue with collision. 🤔

For the rule type label, I'd defer to @maxhoheiser: what do you think?

vittoriopolverino · 2025-03-14T12:32:35Z

Could you please provide some guidance on how to trigger these event rule invocations locally? This would allow me to test the changes end-to-end, send events to Tinybird, and give you a chance to run queries on the test data before merging the PR. That way, we’ll have time to adjust the counter and refine the labels if needed 💪🏼

You can run the queries in the playground I shared with you: Playground

bentsku

Good point Vittorio about the testing instructions. I think running an integration test with against a manually launched LocalStack should do it. I'll update the PR description 😄

bentsku · 2025-03-14T18:04:16Z

localstack-core/localstack/services/events/provider.py

+                        rule_invocation.labels(
+                            status=InvocationStatus.error,
+                            service=target_sender.service,
+                        )


good point, maybe just the exception type? I'm not sure how many different types we can have, and also in this sense we'd probably need 2 different counters, as the labels would be different for both.

I don't have a lot of context into those and what we actually need for EventBridge, so if you want to add on this PR, you can 😄

bentsku · 2025-03-14T18:05:47Z

localstack-core/localstack/services/events/analytics.py

+# number of EventBridge rule invocations per target (e.g., aws:lambda)
+# - status label can be `success` or `error`, see InvocationStatus
+# - service label is the target service name
+rule_invocation = Counter(namespace="events", name="rule_invocations", labels=["status", "service"])


Good point for events, it is the true name of the service so if we were to do a general dashboard based on the list of the official service names, that could be easier, but I can also see the issue with collision. 🤔

For the rule type label, I'd defer to @maxhoheiser: what do you think?

bentsku · 2025-03-19T17:27:09Z

@vittoriopolverino @maxhoheiser the PR has been approved, but there were some comments with some open questions. To avoid changing the "schema" of the sent events, do you think we should follow up on those questions now, or do you deem them closed? Thanks!

vittoriopolverino · 2025-03-19T18:14:17Z

Thanks for following up on this, @bentsku , I’d prefer to clear up any doubts now before merging 🙏🏼

Sorry for bringing this up so late 😭 but..... I still have some doubts about the naming. Using "events" as a namespace doesn’t seem very intuitive to me, as it’s actually a subset of EventBridge. If we were to monitor other EventBridge functionalities in the future, "events" wouldn’t really fit under the same umbrella as the other metrics where namespace="eventbridge" 🤔

bentsku · 2025-03-19T18:37:33Z

@vittoriopolverino this was already addressed with 7ed38c2, the namespace is now eventbridge 😄

but the true name of the service in AWS (like in the AWS CLI) really is events and not EventBridge, for example aws events put-events .... So if we were to build automatic dashboards for all services, events will need a special rule.

It is true that in AWS, the EventBridge naming is an umbrella term for many services: Pipes, Scheduler and such.

Maybe if the namespace is EventBridge, then the name field needs to have the events part to it?

vittoriopolverino · 2025-03-19T19:06:07Z

After a quick chat with @bentsku 🙏🏼 , we decided to follow the existing convention already used in the code and repository structure, choosing not to group pipes, events, scheduler, etc., under the EventBridge namespace.

This reverts commit 0c88293.

bentsku added aws:events Amazon EventBridge semver: patch Non-breaking changes which can be included in patch releases labels Mar 13, 2025

bentsku added this to the 4.3 milestone Mar 13, 2025

bentsku self-assigned this Mar 13, 2025

bentsku requested a review from vittoriopolverino March 13, 2025 16:15

bentsku marked this pull request as ready for review March 13, 2025 16:15

bentsku requested review from maxhoheiser and joe4dev as code owners March 13, 2025 16:15

maxhoheiser approved these changes Mar 14, 2025

View reviewed changes

vittoriopolverino approved these changes Mar 14, 2025

View reviewed changes

vittoriopolverino reviewed Mar 14, 2025

View reviewed changes

bentsku commented Mar 14, 2025

View reviewed changes

bentsku force-pushed the update-event-ruler-counter branch from 66a4532 to 7ed38c2 Compare March 14, 2025 18:47

bentsku added 3 commits March 19, 2025 20:54

migrate to new Counter type for Event Rule invocation analytics

9fa8689

update namespace

0c88293

Revert "update namespace"

622ad14

This reverts commit 0c88293.

bentsku force-pushed the update-event-ruler-counter branch from 7ed38c2 to 622ad14 Compare March 19, 2025 19:55

bentsku merged commit 7f97b18 into master Mar 19, 2025
31 checks passed

bentsku deleted the update-event-ruler-counter branch March 19, 2025 20:51

Uh oh!

Events: migrate to new Counter type for Event Rule invocation analytics #12388

Events: migrate to new Counter type for Event Rule invocation analytics #12388

Uh oh!

Conversation

bentsku commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Testing

Uh oh!

github-actions bot commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LocalStack Community integration with Pro

Uh oh!

maxhoheiser left a comment

Choose a reason for hiding this comment

Uh oh!

maxhoheiser Mar 14, 2025

Choose a reason for hiding this comment

Uh oh!

bentsku Mar 14, 2025

Choose a reason for hiding this comment

Uh oh!

vittoriopolverino left a comment

Choose a reason for hiding this comment

Uh oh!

vittoriopolverino Mar 14, 2025

Choose a reason for hiding this comment

Uh oh!

vittoriopolverino Mar 14, 2025

Choose a reason for hiding this comment

Uh oh!

bentsku Mar 14, 2025

Choose a reason for hiding this comment

Uh oh!

vittoriopolverino commented Mar 14, 2025

Uh oh!

bentsku left a comment

Choose a reason for hiding this comment

Uh oh!

bentsku Mar 14, 2025

Choose a reason for hiding this comment

Uh oh!

bentsku Mar 14, 2025

Choose a reason for hiding this comment

Uh oh!

bentsku commented Mar 19, 2025

Uh oh!

vittoriopolverino commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bentsku commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vittoriopolverino commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bentsku commented Mar 13, 2025 •

edited

Loading

github-actions bot commented Mar 13, 2025 •

edited

Loading

vittoriopolverino commented Mar 19, 2025 •

edited

Loading

bentsku commented Mar 19, 2025 •

edited

Loading

vittoriopolverino commented Mar 19, 2025 •

edited

Loading