-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
[SFN] Add usage analytics for Variable Workflow and JSONata features #11963
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
# Successful invocations (also including expected error cases in line with AWS behaviour) | ||
invocation_counter = UsageCounter("stepfunctions:invocation") | ||
|
||
# Unexpected errors that we do not account for | ||
error_counter = UsageCounter("stepfunctions:error") |
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.
Currently unused. Will be implemented in a follow-up
def visitVariable_sample(self, ctx: ASLParser.Variable_sampleContext): | ||
self.has_variable_sampling = True | ||
|
||
def visitAssign_decl(self, ctx: ASLParser.Assign_declContext): | ||
self.has_variable_sampling = True |
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.
We want to set this flag to true if there is an Assign
or if any variable sampling has been used.
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.
Looking good from a SFN standpoint!
self.has_variable_sampling = False | ||
|
||
def visitQuery_language_decl(self, ctx: ASLParser.Query_language_declContext): | ||
query_language_mode_int = ctx.children[-1].getSymbol().type |
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.
You can probably prune this logic if the flag is already set
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.
Done!
LocalStack Community integration with Pro 2 files ± 0 2 suites ±0 34m 42s ⏱️ - 1h 15m 29s Results for commit 3cf219b. ± Comparison against base commit a47e701. This pull request removes 2511 tests.
This pull request skips 1 test.
♻️ This comment has been updated with latest results. |
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.
yeah data 👍
I would skip the try/catch on the metric counter unless we have a good reason from the SFN analyser side.
analyser = UsageMetricsStaticAnalyser() | ||
analyser.analyse(definition=definition) | ||
|
||
try: |
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.
Do we need the try/catch here? has_jsonata
is just a getter and we can expect the counters to work (we're not using try/catch anywhere else)
if we want to be as defensive, then we should rather cover the analyse
step.
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.
We didn't want the metrics collection to be the reason why SFN was failing. Will just surround the whole block in a try-catch rather
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.
OK wrapped the whole process in a try-except
929d29f
to
e4a8071
Compare
Motivation
This PR adds analytics the SFN provider to help ascertain usage of the new JSONata and Variable workflows.
Changes
Testing