Skip to content

Conversation

RanVaknin
Copy link
Contributor

@RanVaknin RanVaknin commented Jul 23, 2025

Motivation and Context

This implementation uses SLF4J MDC (which uses ThreadLocal under the hood) to read the trace ID value stored in AWS_LAMBDA_X_TraceId that would be set by the Lambda runtime (this would be added later, probably here).

@RanVaknin RanVaknin added the changelog-not-required Indicate changelog entry is not required for a specific PR label Jul 23, 2025
@RanVaknin RanVaknin requested a review from a team as a code owner July 23, 2025 23:17
Copy link
Contributor

@millems millems left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will async clients be addressed in a separate PR? This won't work for async retries (which are delegated to a retries thread pool), calls that happen within future chaining (which are delegated to a future completion executor thread pool), etc.

We'll want to find where calls might switch threads (i.e. anywhere we're using futures) and then preserve the thread local setting across that thread change.

@RanVaknin RanVaknin changed the title Add support for lambda traces for concurrent environments Add support for lambda traces via threadLocal Jul 25, 2025
.responseBody(AbortableInputStream.create(new StringInputStream("{}")))
.build());

client.allTypes().join();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also add a test sending a request in the result future:

client.allTypes()
      .thenRun(() -> client.allTypes() /* Did this request have the right trace ID? */)

I suspect we'll need to plumb the thread local context to there as well (and clear it, so that reuses of that thread don't have it set).

Copy link

@RanVaknin
Copy link
Contributor Author

flakey integ test is the only failure S3TransferManagerDownloadPauseResumeIntegrationTest Merging.

@RanVaknin RanVaknin enabled auto-merge August 12, 2025 18:21
@RanVaknin RanVaknin added the no-api-surface-area-change Indicate there is no API surface area change and thus API surface area review is not required label Aug 13, 2025
@RanVaknin RanVaknin added this pull request to the merge queue Aug 13, 2025
Merged via the queue into master with commit 038534c Aug 13, 2025
43 of 59 checks passed
Copy link

This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
changelog-not-required Indicate changelog entry is not required for a specific PR no-api-surface-area-change Indicate there is no API surface area change and thus API surface area review is not required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants