-
Notifications
You must be signed in to change notification settings - Fork 1k
feat: Aws Lambda Request Handler instrumentation uses OTEL_INSTRUMENTATION_AWS_LAMBDA_FLUSH_TIMEOUT
#12375
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
Conversation
| } | ||
|
|
||
| OpenTelemetrySdkAccess.forceFlush(1, TimeUnit.SECONDS); | ||
| OpenTelemetrySdkAccess.forceFlush(flushTimeoutNanos, TimeUnit.NANOSECONDS); |
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.
| OpenTelemetrySdkAccess.forceFlush(flushTimeoutNanos, TimeUnit.NANOSECONDS); | |
| OpenTelemetrySdkAccess.forceFlush(flushTimeout(), TimeUnit.SECONDS); |
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.
OTEL_LAMBDA_FLUSH_TIMEOUT_DEFAULT is 10 seconds.
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.
Yes, OTEL_LAMBDA_FLUSH_TIMEOUT_DEFAULT is 10, that is a braking change from 1 second.
How to add proper changelog?
| @SuppressWarnings("unused") | ||
| public static class HandleRequestAdvice { | ||
|
|
||
| private final static long flushTimeoutNanos = flushTimeout().toNanos(); |
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.
| private final static long flushTimeoutNanos = flushTimeout().toNanos(); |
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 have intentionally matched the reference implementation from https://github.com/search?q=repo%3Aopen-telemetry%2Fopentelemetry-java-instrumentation+flushTimeout.toNanos&type=code
Made it static as it is unlikely to change within the runtime, but I can make it non-static and non-final attribute if that will help.
…TATION_AWS_LAMBDA_FLUSH_TIMEOUT` Using `WrapperConfiguration.flushTimeout()` for consistency with `TracingRequestStreamWrapper`
90c8a46 to
757a154
Compare
| @SuppressWarnings("unused") | ||
| public static class HandleRequestAdvice { | ||
|
|
||
| private static final long flushTimeoutNanos = flushTimeout().toNanos(); |
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.
We don't add fields to advice classes. Either inline it or move the field to https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/aws-lambda/aws-lambda-core-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awslambdacore/v1_0/AwsLambdaInstrumentationHelper.java
It would be best to keep changes here aligned with
Line 109 in 4497fbf
| OpenTelemetrySdkAccess.forceFlush(1, TimeUnit.SECONDS); |
|
superseded by #12576 |
WrapperConfiguration.flushTimeout()for consistency withTracingRequestStreamWrapperOTEL_LAMBDA_FLUSH_TIMEOUTenv varOTEL_LAMBDA_FLUSH_TIMEOUT_DEFAULTthe flush timeout is now 10 seconds, was 1 second