-
Notifications
You must be signed in to change notification settings - Fork 96
feat: Support CRaC priming for powertools-tracing and powertools-serialization #2104
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
feat: Support CRaC priming for powertools-tracing and powertools-serialization #2104
Conversation
…alization - Add CRaC dependency and generate-classesloaded-file profile to both modules - Implement Resource interface in TracingUtils and JsonConfig classes - Add classesloaded.txt files for automatic class preloading - Add comprehensive CRaC tests for both modules - Update documentation with SnapStart priming guidance - Update spotbugs-exclude.xml for beforeCheckpoint methods Addresses issues aws-powertools#2004 and aws-powertools#2003
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.
Hey @dcabib,
thanks for sending this PR. I added a couple of comments.
After implemented, I will test this in my AWS account to see if the runtime hooks get correctly executed at runtime when publishing a SnapStart enabled Lambda version using these dependencies.
getObjectMapper(); | ||
getJmesPath(); |
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.
This is not needed since we load it below anyway.
try { | ||
ObjectMapper mapper = getObjectMapper(); | ||
|
||
// Prime common AWS Lambda event types as suggested by Philipp | ||
primeEventType(mapper, "com.amazonaws.services.lambda.runtime.events.APIGatewayProxyRequestEvent", | ||
"{\"httpMethod\":\"GET\",\"path\":\"/test\",\"headers\":{\"Content-Type\":\"application/json\"}}"); | ||
primeEventType(mapper, "com.amazonaws.services.lambda.runtime.events.APIGatewayV2HTTPEvent", | ||
"{\"version\":\"2.0\",\"routeKey\":\"GET /test\",\"requestContext\":{\"http\":{\"method\":\"GET\"}}}"); | ||
primeEventType(mapper, "com.amazonaws.services.lambda.runtime.events.SQSEvent", | ||
"{\"Records\":[{\"messageId\":\"test\",\"body\":\"test message\"}]}"); | ||
primeEventType(mapper, "com.amazonaws.services.lambda.runtime.events.SNSEvent", | ||
"{\"Records\":[{\"Sns\":{\"Message\":\"test message\",\"TopicArn\":\"arn:aws:sns:us-east-1:123456789012:test\"}}]}"); | ||
primeEventType(mapper, "com.amazonaws.services.lambda.runtime.events.KinesisEvent", | ||
"{\"Records\":[{\"kinesis\":{\"data\":\"dGVzdA==\",\"partitionKey\":\"test\"}}]}"); | ||
primeEventType(mapper, "com.amazonaws.services.lambda.runtime.events.ScheduledEvent", | ||
"{\"source\":\"aws.events\",\"detail-type\":\"Scheduled Event\"}"); | ||
|
||
// Warm up JMESPath function registry | ||
getJmesPath().compile("@").search(mapper.readTree("{\"test\":\"value\"}")); | ||
|
||
} catch (Exception e) { | ||
// Ignore exceptions during priming as they're expected in some environments | ||
} |
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.
This doesn't have any side-effects and we should aim at not catching an exception at all.
- Can you generate realistic events that pass Jackson loading?
- Why are you loading the event classes reflectively using
Class<?> eventClass = Class.forName(className);
? I don't think this is necessary – let's import them directly and parse the generated fake events usingmapper.readValue(fakeEvent, SQSEvent.class)
.
I think you can use sam cli to generate fake events https://docs.aws.amazon.com/serverless-application-model/latest/developerguide/sam-cli-command-reference-sam-local-generate-event.html
@Test | ||
void testBeforeCheckpointDoesNotThrowException() { | ||
assertThatNoException().isThrownBy(() -> config.beforeCheckpoint(context)); | ||
} | ||
|
||
@Test | ||
void testAfterRestoreDoesNotThrowException() { | ||
assertThatNoException().isThrownBy(() -> config.afterRestore(context)); | ||
} |
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.
After removing the exception wildcard catch these tests should still pass. Right now, they have no effect.
See comment: https://github.com/aws-powertools/powertools-lambda-java/pull/2104/files#r2314166695
} catch (Exception e) { | ||
// Ignore exceptions during priming as they're expected in some environments | ||
LOG.debug("Exception during X-Ray priming (expected in some environments): {}", e.getMessage()); | ||
} |
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.
Can you check if we can remove this exception catching as well?
// Preload classes | ||
ClassPreLoader.preloadClasses(); |
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.
Let's move pre-load classes to the top of beforeCheckpoint
everywhere? This makes sure that this always runs even if something else throws an exception at runtime. It has dedicated unit tests making sure that this is reliable.
// Access the private INSTANCE field using reflection | ||
Field instanceField = TracingUtils.class.getDeclaredField("INSTANCE"); | ||
instanceField.setAccessible(true); | ||
TracingUtils tracingUtils = (TracingUtils) instanceField.get(null); |
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.
Why is this necessary?
<And> | ||
<Class name="software.amazon.lambda.powertools.utilities.JsonConfig"/> | ||
<Method name="beforeCheckpoint"/> | ||
</And> |
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.
I believe we can remove this again after implementing my comment about JsonConfig
priming. If it is still needed let's keep it there.
docs/core/tracing.md
Outdated
=== "Constructor" | ||
|
||
```java hl_lines="7" | ||
import software.amazon.lambda.powertools.tracing.Tracing; | ||
import software.amazon.lambda.powertools.tracing.TracingUtils; | ||
|
||
public class MyFunctionHandler implements RequestHandler<APIGatewayProxyRequestEvent, APIGatewayProxyResponseEvent> { | ||
|
||
public MyFunctionHandler() { | ||
TracingUtils.putAnnotation("init", "priming"); // Ensure TracingUtils is loaded for SnapStart | ||
} | ||
|
||
@Override | ||
@Tracing | ||
public APIGatewayProxyResponseEvent handleRequest(APIGatewayProxyRequestEvent input, Context context) { | ||
// ... | ||
return something; | ||
} | ||
} | ||
``` | ||
|
||
=== "Static Initializer" | ||
|
||
```java hl_lines="7" | ||
import software.amazon.lambda.powertools.tracing.Tracing; | ||
import software.amazon.lambda.powertools.tracing.TracingUtils; | ||
|
||
public class MyFunctionHandler implements RequestHandler<APIGatewayProxyRequestEvent, APIGatewayProxyResponseEvent> { | ||
|
||
static { | ||
TracingUtils.putAnnotation("init", "priming"); // Ensure TracingUtils is loaded for SnapStart | ||
} | ||
|
||
@Override | ||
@Tracing | ||
public APIGatewayProxyResponseEvent handleRequest(APIGatewayProxyRequestEvent input, Context context) { | ||
// ... | ||
return something; | ||
} | ||
} | ||
``` |
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.
I think we should expand TracingUtils
with a method TracingUtils.prime()
that has no side-effects unlike the example given here. As a user, I don't want to be forced to add e.g. an annotation to my traces just to trigger SnapStart priming.
- Add TracingUtils.prime() method with no side-effects for public API - Move ClassPreLoader.preloadClasses() to top of beforeCheckpoint methods - Remove unnecessary exception catching in CRaC hooks - Update JsonConfig to use direct imports instead of reflection for AWS Lambda events - Fix CRaC tests to not use reflection for accessing private fields - Update documentation examples to use TracingUtils.prime() - Consolidate SpotBugs exclusions into single Or structure All CRaC tests passing (4 tests, 0 failures)
|
Hey @dcabib, if possible, let's split this PR in two PRs please since it addresses two separate issues. It will be easier to address comments on a smaller scale since I see some of my comments were ignored in the recent push and all of them were left unanswered. |
Closing this PR as requested by @phipag to split into separate PRs. This has been addressed with:
Both separate PRs are now ready for review with all SonarQube issues resolved and all tests passing. |
Summary
Adds support for CRaC priming in both the powertools-tracing and powertools-serialization modules to improve AWS Lambda SnapStart restore durations.
Changes
Implementation Details
Tracing Module:
Serialization Module:
Both implementations follow the exact same patterns established in PR #2081 for the validation module.
Testing
Issue numbers: #2004 and #2003
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.