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

Fix flaky jersey jaxrs async test #6523

Merged
merged 1 commit into from
Sep 2, 2022

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Aug 30, 2022

https://ge.opentelemetry.io/s/tr2jbhq7r4r2g/tests/:instrumentation:jaxrs:jaxrs-2.0:jaxrs-2.0-jersey-2.0:javaagent:test/JerseyJettyHttpServerTest/should%20handle%20failing%20AsyncResponse?expanded-stacktrace=WyIwLTEiXQ&top-execution=1
See tested endpoint

void asyncOp(@Suspended AsyncResponse response, @QueryParam("action") String action) {
CompletableFuture.runAsync({
// await for the test method to verify that there are no spans yet
BARRIER.await(1, SECONDS)
switch (action) {
case "succeed":
response.resume("success")
break
case "throw":
response.resume(new Exception("failure"))
break
case "cancel":
response.cancel()
break
default:
response.resume(new AssertionError((Object) ("invalid action value: " + action)))
break
}
})
}
and test class
def "should handle #desc AsyncResponse"() {
given:
def url = address.resolve("async?action=${action}").toString()
when: "async call is started"
def futureResponse = client.get(url).aggregate()
then: "there are no traces yet"
assertTraces(0) {
}
when: "barrier is released and resource class sends response"
awaitBarrier(1, SECONDS)
def response = futureResponse.join()
then:
response.status().code() == statusCode
bodyPredicate(response.contentUtf8())
def spanCount = 2
def hasSendError = asyncCancelHasSendError() && action == "cancel"
if (hasSendError) {
spanCount++
}
assertTraces(1) {
trace(0, spanCount) {
asyncServerSpan(it, 0, url, statusCode)
handlerSpan(it, 1, span(0), "asyncOp", isCancelled, isError, errorMessage)
if (hasSendError) {
sendErrorSpan(it, 2, span(1))
}
}
}
where:
desc | action | statusCode | bodyPredicate | isCancelled | isError | errorMessage
"successful" | "succeed" | 200 | { it == "success" } | false | false | null
"failing" | "throw" | 500 | { it == "failure" } | false | true | "failure"
"canceled" | "cancel" | 503 | { it instanceof String } | true | false | null
}

For the failed test span should be ended in
public static void stopSpan(
@Advice.This AsyncResponse asyncResponse, @Advice.Argument(0) Throwable throwable) {
VirtualField<AsyncResponse, AsyncResponseData> virtualField =
VirtualField.find(AsyncResponse.class, AsyncResponseData.class);
AsyncResponseData data = virtualField.get(asyncResponse);
if (data != null) {
virtualField.set(asyncResponse, null);
instrumenter().end(data.getContext(), data.getHandlerData(), null, throwable);
}
when response.resume(new Exception("failure")) call exits. Apparently it is possible that this call from CompletableFuture.runAsync thread runs concurrently with the main thread that is exiting the JaxRsTestResource.asyncOp method and the span is ended in
This change makes it so that when a jax-rs method has async response span is only ended when the async response is resumed or canceled. Exiting the jax-rs method will not end the span if the method has an async response.

Hopefully also fixes similar failure in resteasy tests
https://ge.opentelemetry.io/s/be55uszpcc2fk/tests/:instrumentation:jaxrs:jaxrs-3.0:jaxrs-3.0-resteasy-6.0:javaagent:test/ResteasyHttpServerTest/should%20handle%20failing%20AsyncResponse?expanded-stacktrace=WyIwIiwiMC0xIl0&top-execution=1

@laurit laurit requested a review from a team as a code owner August 30, 2022 17:11
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

👍

@laurit laurit merged commit 79acca7 into open-telemetry:main Sep 2, 2022
@laurit laurit deleted the jaxrs-async-race branch September 2, 2022 10:28
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Oct 23, 2022
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Oct 31, 2022
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Dec 4, 2022
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.

None yet

3 participants