Support extracting Trace Header via MDC in Lambda Segment Context #431
Conversation
| return TraceHeader.fromString(lambdaTraceHeaderKey != null && lambdaTraceHeaderKey.length() > 0 | ||
| ? lambdaTraceHeaderKey | ||
| : System.getProperty(LAMBDA_TRACE_HEADER_PROP)); | ||
| String lambdaTraceHeaderKeyFromMdc = MDC.get(CONCURRENT_TRACE_ID_KEY); |
There was a problem hiding this comment.
This slf4j API will return null if there is no implementation for logging (e.g. log4j-slf4j-impl, log4j-core).
I'm not exactly sure what guarantees (if it is guaranteed) the logging implementation to exist in Lambda Environments, but this is how AWS SDK is implementing this logic (which also only depends on slf4j API, not the implementation that is needed like in the unit test dependencies).
There was a problem hiding this comment.
Hi there,
This is correct. For this to work one of the following has to provide this implementation:
- The Lambda runtime
- The AWS Java SDK v2 - not possible because it will break current customers who have their own implementation (logback etc)
- The Xray SDK
- The customer - probably not possible since this is supposed to work by default (?) and this would require customers to explicitly provide an implementation.
|
|
||
| if (lambdaTraceHeaderKeyFromMdc != null && lambdaTraceHeaderKeyFromMdc.length() > 0) { | ||
| return TraceHeader.fromString(lambdaTraceHeaderKeyFromMdc); | ||
| } else if (lambdaTraceHeaderKeyFromEnvVar != null && lambdaTraceHeaderKeyFromEnvVar.length() > 0) { |
There was a problem hiding this comment.
nit: You don't need 'else' as you have 'return' in the previous block.
| // See: https://github.com/aws/aws-xray-sdk-java/issues/251 | ||
| private static final String LAMBDA_TRACE_HEADER_PROP = "com.amazonaws.xray.traceHeader"; | ||
|
|
||
| public static TraceHeader getTraceHeaderFromEnvironment() { |
There was a problem hiding this comment.
Consider rename this function getTraceHeaderFromEnvironment()
| implementation("com.google.auto.value:auto-value-annotations:1.10.4") | ||
| implementation("com.google.auto.service:auto-service-annotations:1.1.1") | ||
| implementation("org.apache.httpcomponents:httpclient:4.5.14") | ||
| implementation("org.slf4j:slf4j-api:1.7.30") |
There was a problem hiding this comment.
May I know why using this old version? Q complains this version has performance issue. https://mvnrepository.com/artifact/org.slf4j/slf4j-api?
lukeina2z
left a comment
lukeina2z
left a comment
There was a problem hiding this comment.
revert previous approval.
|
Closing PR as the solution will be changed - See aws/aws-sdk-java-v2#6366 |
3 participants
Issue #, if available:
Follow pattern used by AWS SDK for Java:
Description of changes:
lambdaTraceHeaderfrom MDC first, then Env Var, then System Property. Before it would just prioritize Env Var then System Property.Other Thoughts:
slf4jto extract from MDC, similarly to AWS SDK. If there is no implementation for logging (e.g.log4j-slf4j-impl,log4j-core), this extraction logic will not work (MDC.get(...)returnsnull) and fallback to Env Var or System Property. I'm not exactly what guarantees (if it is guaranteed) the logging implementation to exist in Lambda Environments.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.