-
Notifications
You must be signed in to change notification settings - Fork 936
Feature/master/utils lite lambda trace #6404
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: master
Are you sure you want to change the base?
Conversation
Add utils-lite package
Add support for concurrent trace id propagation
9badcf1
to
85642fe
Compare
.importPackages("software.amazon.awssdk.utilslite"); | ||
|
||
@Test | ||
public void utilsLitePackage_shouldOnlyContainAllowedClasses() { |
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.
Is this a requirement ?
Why is this test required ?
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 was following @zoewangg suggestion to add a manual check to make sure we don't add classes to this package unintentionally. The sole purpose of this package as of now is to add this wrapper. If we want to expand this in the future, this presents another checkpoint.
private static void saveTraceId(ExecutionAttributes executionAttributes) { | ||
String traceId = executionAttributes.getAttribute(TRACE_ID); | ||
if (traceId != null) { | ||
SdkInternalThreadLocal.put(CONCURRENT_TRACE_ID_KEY, executionAttributes.getAttribute(TRACE_ID)); |
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 see SdkInternalThreadLocal we do a put and we never call remove/clear thus if TRACE_ID
is unique then the map in SdkInternalThreadLocal will keep growing . How do we clear/remove CONCURRENT_TRACE_ID_KEY for request that are compete
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.
If a thread goes back to thread pool, and a fresh request is being fired from the Lambda Runtime Client interface, it will contain a brand new traceID stored under the same key on the map so it will overwrite itself.
If the thread is killed, then when a new thread is being spun up, a clean map is created.
utils-lite/src/main/java/software/amazon/awssdk/utilslite/SdkInternalThreadLocal.java
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
public void beforeExecution(Context.BeforeExecution context, ExecutionAttributes executionAttributes) { | ||
String traceId = SdkInternalThreadLocal.get(CONCURRENT_TRACE_ID_KEY); |
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.
Since we are using ThreadLocal can we confirm if this can come across virtual thread ?
As in if customer makes call on a virtual thread , if the impact has been investigated ?
public void beforeExecution(Context.BeforeExecution context, ExecutionAttributes executionAttributes) { | ||
String traceId = SdkInternalThreadLocal.get(CONCURRENT_TRACE_ID_KEY); | ||
if (traceId != null) { | ||
executionAttributes.putAttribute(TRACE_ID, traceId); |
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.
Also I was thinking about this case where client is running on host where SdkInternalThreadLocal was not cleared/not-used
1. beforeExecution():
- SdkInternalThreadLocal.get(CONCURRENT_TRACE_ID_KEY) → null (first time)
- Creates HashMap for Thread-1 (empty)
Now lets imagine the user is creating new request for every thread as below
for (int i = 0; i < 1_000_000; i++) {
new Thread(() -> {
AmazonS3 client = AmazonS3ClientBuilder.standard().build();
client.getObject(...); // Creates HashMap for this thread
client.shutdown();
// Thread dies, but not sure what will happen to the above empty HashMap instantaited
}).start();
}
Result: 1,000,000 HashMaps created ? Can we please confirm this will not cause any leak of empty hash maps ?
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.
Great call out. I didnt think about this. I changed it so that the hashmap only gets initialized when .put()
is being called (initially called by lambda RIC)
|
This PR adds:
SdkInternalThreadLocal
, a wrapper around threadLocal.Background
Previously, we implemented trace ID propagation using SLF4J's MDC in PR #6363, but this was
reverted because the MDC interface exists but the implementation is not provided by the SDK, Lambda runtime, or X-Ray SDK.
Solution
Added a small
utils-lite
utility class that provides thread local key value storage usingThreadLocal<Map<String, String>>
. For this case, it allows the Lambda Runtime Interface Client, AWS SDK, and X-Ray SDK to share trace context via this one package, but can extended to other use cases.Example: