-
Notifications
You must be signed in to change notification settings - Fork 92
feat: Support CRaC and priming of powertools metrics and idempotency-dynamodb #1861
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
base: main
Are you sure you want to change the base?
feat: Support CRaC and priming of powertools metrics and idempotency-dynamodb #1861
Conversation
@phipag Looks like v2 is now merged and released, so I resolved conflicts to rebase to |
Thanks @subhash686 and apologize the delay. I was very busy with the v2 release. Your PR is next on my list and we'll try to get it merged for the next minor release 2.1.0. |
Hey @subhash686, I didn't forget you. This week is very busy but I'll do my best to test your implementation very soon. In the meantime, can you have a look at the 4 Sonar issued and either resolve them or mark them as not applicable if you think that is the case? |
@phipag Fixed the sonar issues, I did not notice the comment earlier, thanks for pointing it out. |
Hey @subhash686 , I am reviewing your PR now. Can you provide some details how you generated the txt file of the classes loaded? I see you say:
Can you provide the compilation command you used for this? Ideally, I would like to reproduce the process for how to generate and then use the txt file of loaded classes. |
Thanks @phipag . Here are the steps I followed
|
Thanks @subhash686 for providing this context. I will reproduce it and run a load test of this code. Will share the results here soon and we can move forward. |
Hey @subhash686, here is my first update. I tested the classesload.txt (automatic priming) implementation that you contributed and found some issues:
For issue (2), I think this needs a completely different approach for generating the classesloaded.txt. We need to implement a mechanism to capture the classes at runtime not the classes used by maven during compilation. Possibly, the GraalVM mechanism can be re-used in some form? I will now review the Idempotency manual priming that you implemented and will get back to you again. Just wanted to share these preliminary findings. Let me know if you have any suggestion / idea about this. I am happy to join a meeting with you to brainstorm more? |
replaced build time class list with run time class list
Hi @phipag For (2), I created a lambda handler with FlushMetrics annotation and used it in a test to generated the classes again. Used GraalVM profile to run all tests with the -Xlog JVM option. I see the runtime classes now with some extra test classes. I will fix (1) once you get a chance to review it all. |
public void afterRestore(org.crac.Context<? extends Resource> context) throws Exception { | ||
// This is a no-op, as we don't need to do anything after restore | ||
} | ||
|
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 we move this to the DynamoDBPersistenceStore.java? It will not run when included here because this class is only used from the AspectJ Aspect.
Also, don't forget to register the CraC resource in the constructor of the class.
From the DynamoDBPersistenceStore we can then run some more logic such as createRecord, updateRecord, and getRecord:
/**
* Primes the persistent store by invoking the get record method with a key that doesn't exist.
*
* @param context
* @throws Exception
*/
@Override
public void beforeCheckpoint(org.crac.Context<? extends Resource> context) throws Exception {
try {
System.out.println("beforeCheckpoint");
this.getRecord("__invoke_prime__");
// Add logic for createRecord, updateRecord here and catch exceptions
} catch (IdempotencyItemNotFoundException keyNotFound) {
// This is expected, as we are priming the store
System.out.println("beforeCheckpoint IdempotencyItemNotFoundException");
} catch (Exception unknown) {
// This is unexpected but we must continue without any interruption
System.out.println("beforeCheckpoint Exception");
}
}
@Override
public void afterRestore(org.crac.Context<? extends Resource> context) throws Exception {
// This is a no-op, as we don't need to do anything after restore
System.out.println("afterRestore");
}
<dependency> | ||
<groupId>org.crac</groupId> | ||
<artifactId>crac</artifactId> | ||
</dependency> |
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 also move this dependency to the dynamodb module for now.
Hey @subhash686, I reviewed and tested the DynamoDB priming (see my comments above). I made these changes locally and ran a load test with your priming code and without the priming code. I observe positive results that confirm priming actually improves the restore duration. Here are my results: Idempotency Snapstart restore durationTested using example at https://github.com/aws-powertools/powertools-lambda-java/blob/984036ca2e3afb92b2c838bcfa2e48a51c3d7a64/examples/powertools-examples-idempotency Priming achieves ~27% better restore duration. Without priming
With priming
Nice, the file looks much better now although it also includes the unit test libraries like
Happy to hear that. Let's setup a meeting to brainstorm the best approach together for this class loading. Can you send an email to |
I performed the same load test for the Classloading using the new txt file you provided @subhash686. Here are my results: Metrics priming (Classloading)Tested using example at https://github.com/aws-powertools/powertools-lambda-java/blob/984036ca2e3afb92b2c838bcfa2e48a51c3d7a64/examples/powertools-examples-core-utilities/sam (removed tracer and logger). We can see ~7% better performance using classloading priming. Without priming
With priming
|
Next steps:
|
|
Issue #, if available:
#1588
Description of changes:
Checklist
Breaking change checklist
RFC issue #:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.