Skip to content
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

Unify aws lambda flush handling #12576

Merged
merged 4 commits into from
Nov 6, 2024
Merged

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
@@ -7,7 +7,7 @@ This package contains libraries to help instrument AWS lambda functions in your
To use the instrumentation, configure `OTEL_INSTRUMENTATION_AWS_LAMBDA_HANDLER` env property to your lambda handler method in following format `package.ClassName::methodName`
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

}

private static volatile ForceFlusher forceFlush;

/** 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