Skip to content

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Nov 6, 2024

Supersedes #12375
Set default flush timeout for agent instrumentation to 10s as it is for the wrapper used in the library configuration.

@laurit laurit requested a review from a team as a code owner November 6, 2024 11:58
@github-actions github-actions bot requested a review from theletterf November 6, 2024 11:59
and use one of wrappers as your lambda `Handler`.

In order to configure a span flush timeout (default is set to 1 second), please configure `OTEL_INSTRUMENTATION_AWS_LAMBDA_FLUSH_TIMEOUT` env property. The value is in seconds.
In order to configure a span flush timeout (default is set to 10 second), please configure `OTEL_INSTRUMENTATION_AWS_LAMBDA_FLUSH_TIMEOUT` env property. The value is in milliseconds.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell this is configured in

public static final Duration OTEL_LAMBDA_FLUSH_TIMEOUT_DEFAULT = Duration.ofSeconds(10);
public static Duration flushTimeout() {
String lambdaFlushTimeout = System.getenv(OTEL_LAMBDA_FLUSH_TIMEOUT_ENV_KEY);
if (lambdaFlushTimeout != null && !lambdaFlushTimeout.isEmpty()) {
try {
return Duration.ofMillis(Long.parseLong(lambdaFlushTimeout));
} catch (NumberFormatException nfe) {
// ignored - default used
}
}
return OTEL_LAMBDA_FLUSH_TIMEOUT_DEFAULT;
}


| System property | Type | Default | Description |
|-------------------------------------------------|---------|---------|--------------------------------|
| `otel.instrumentation.aws-lambda.flush-timeout` | Integer | 10000 | Flush timeout in milliseconds. |
Copy link
Contributor

@steverao steverao Nov 6, 2024

Choose a reason for hiding this comment

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

How about using Long instead of Integer here? in java.util.concurrent.TimeUnit, I noticed it used long type for millisecond or other unit of time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it really matter? From user perspective I'd assume it is only important to know that a numeric value is expected.

Copy link
Member

Choose a reason for hiding this comment

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

for configuration, I think we only need to specify something like "int" or "double", similar to semantic convention attribute types


/** Forces flushing of pending spans. */
public static void forceFlush(int timeout, TimeUnit unit) {
public static void forceFlush(long timeout, TimeUnit unit) {
Copy link
Member

Choose a reason for hiding this comment

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

maybe keep and mark the int variant deprecated for removal in 3.0? seems like something that could be in use by an extension

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added back the original method. If the indy extension changes get done in time for 3.0 it is possible that we won't need most of the bootstrap classes any more.

@trask trask enabled auto-merge (squash) November 6, 2024 18:39
@trask trask merged commit 6a9654d into open-telemetry:main Nov 6, 2024
56 checks passed
akats7 pushed a commit to akats7/opentelemetry-java-instrumentation that referenced this pull request Nov 21, 2024
akats7 pushed a commit to akats7/opentelemetry-java-instrumentation that referenced this pull request Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants