-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Migrate legacy usage counters to new analytic counters #12412
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
LocalStack Community integration with Pro 2 files ± 0 2 suites ±0 1h 30m 8s ⏱️ - 25m 23s Results for commit e5bb66c. ± Comparison against base commit 68df8dc. This pull request removes 1162 tests.
♻️ This comment has been updated with latest results. |
I’d suggest yes if
That’s a tough one, and it ties back to question (a) 🤣 If However, if you anticipate that different operations will diverge and require operation-specific labels, then I’d suggest creating two separate metrics instead of using an operation label, even if the two metrics share some common characteristics.
What’s the cardinality here, this? If this is the cardinality, it seems reasonable to track it, as long as it’s valuable for you. Additionally, if everything Additional Note: That said, I wouldn’t want this to be over-relied upon or used as a fallback for every case. Ideally, we should still aim to anticipate future needs as much as possible. If you’d like, we can have a quick chat and run some queries together to test different combinations or possibilities. That might give me a better understanding of your intentions and help refine the approach 💪🏼 |
@vittoriopolverino to confirm, do you suggest here to have 2 separate labels - one for general operation result, i.e. success/error, and another one for specific reason for error? |
@joe4dev @vittoriopolverino I added a draft PR for EventBridge Pipes: https://github.com/localstack/localstack-ext/pull/4224 |
Yes 👍🏼
|
From a data modeling perspective, if there are enums having the same meaning in both EventBridge Pipes and ESM, it would make sense to ensure consistency across them. That way, you can use the same filter to reference a status across both Pipes and ESM, instead of needing two separate filters to represent the same status. (Not sure if this kind of cross-service analysis is something you’d ever do, but in any case, if we can ensure consistency, it’s better) |
One of the main question for me is what do we categorize as "error":
@vittoriopolverino @tiurin What is your understanding/suggestion regarding what should be considered an "error"? |
🤔 If an error is expected (part of intended application behavior), doesn’t require intervention, and isn’t something we need to act on, then maybe we need a way to differentiate errors. We have two options: Which approach makes the most sense really depends on the query patterns you/we anticipate using the most in the future. I don’t think there’s a strictly right or wrong choice between the two, it’s more about what will make querying and analysis more efficient for your use cases |
Pipes is a more broad case of ESM, moreover they have some shared code, so if we can align both well we will avoid "if pipes" conditions for counters. |
a5a534b
to
2bb900b
Compare
2bb900b
to
cfb8d1e
Compare
Background on trade-off discussions:
Most dimensions seem relevant for both
In ESM, Pipes, and Lambda, it's not clearly possible to distinguish actionable and user-side errors (e.g., invocation_error can have multiple root causes). Therefore, we couldn't clearly identify high-level error categories and a boolean flag |
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.
ESM part ✅ 🚀
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 🥳 Fantastic work, really appreciate the deep thinking that went into using labels effectively. Some great discussions, questions, and insights came up while applying the new counter and structuring the labels 🚀
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.
Don't we need a similar or "n/a"
for the invokes in the function counters as well, not just the creation?
absolutely spot on @dfangl 💯 Thank you! I added the |
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. I think we should consider adding a counter for cold vs. warm starts since it appears to have an influence on performance of ESM. Not necessary in this PR though 🙂
Motivation
Migrate Lambda and Lambda Event Source Mapping counters to our new standard.
Changes
n/a
ifpackage_type
isImage
Testing
aws.services.lambda_.test_lambda_developer_tools.TestHotReloading.test_hot_reloading
aws.services.lambda_.test_lambda.TestLambdaFeatures.test_invocation_type_request_response
aws.services.lambda_.test_lambda.TestLambdaFeatures.test_invocation_type_event
aws.services.lambda_.test_lambda_common.TestLambdaRuntimesCommon.test_echo_invoke
test_lambda.py
tests/aws/services/lambda_/event_source_mapping
Questions
a) hotreload>operation: Should we use the operations label (e.g., create) if we envison that at some point we might want to track other operations (e.g., reload). Unfortunately, that's a bit tricky in this case because we would need to do that in our custom Lambda RIE Golang codebase.
b) function>operation: Should we add an operations label (e.g., invoke) if we envison to track create as well? If so, how to treat the invocation type label, which only applies for the invoke operation?
c) function>status: What do you think about using typed definitions for such a status label? Are we too fine-grained here?