[Bug] apm-jdk-http-plugin leads to duplicated traceid

[zhaoxiaojie0415]

Search before asking

• I had searched in the issues and found no similar issues.

Apache SkyWalking Component

Java Agent (apache/skywalking-java)

What happened

In apm-jdk-http-plugin, it creates span brefore 'writeRequests' method in sun.net.www.http.HttpClient and stop span after 'parseHTTP' method. However, within the writeRequest method, if a request fails, the writeRequest method might be called again for retry purposes.

What you expected to happen

How to reproduce

public static InputStream getInputStream(String urlPath) {
InputStream inputStream = null;
HttpURLConnection httpURLConnection = null;
try {
URL url = new URL(urlPath);
httpURLConnection = (HttpURLConnection) url.openConnection();

        httpURLConnection.setConnectTimeout(3000);
       
        httpURLConnection.setDoInput(true);
        httpURLConnection.setRequestMethod("GET");
        int responseCode = httpURLConnection.getResponseCode();
        log.info("responseCode is: {}", responseCode);
        if (responseCode == HttpURLConnection.HTTP_OK) {
            
            inputStream = httpURLConnection.getInputStream();
        } else {
            inputStream = httpURLConnection.getErrorStream();
        }
    } catch (MalformedURLException e) {
        log.info("malformedURLException while get inputStream, url = {}", urlPath, e);
        throw new RuntimeException();
    } catch (IOException e) {
        log.info("IOException while get inputStream, url = {}", urlPath, e);
        throw new RuntimeException();
    }
    return inputStream;
}

To reproduce the issue by simulating network packet loss, we can use tools like tc (Traffic Control) on Linux and run the code above.

Anything else

No response

Are you willing to submit a pull request to fix on your own?

• Yes I am willing to submit a pull request on my own!

Code of Conduct

• I agree to follow this project's Code of Conduct

[wu-sheng]

This is a common edge case. The retry mechanism is always bringing new challenges. What do you propose as you clicked willing to fix?

[zhaoxiaojie0415]

This is a common edge case. The retry mechanism is always bringing new challenges. What do you propose as you clicked willing to fix?

I think we can try to track whether the current thread has already created a span (in apm-jdk-http-plugin scope). If it has already been created, we will not create a new one. This way, when the writeRequest method is called again (e.g., during retries), duplicate spans won't be created for the same operation.

[wu-sheng]

Do you mean to check the current span? How do you verify whether a span is created? I am not sure.

In theory, every retry should create a span, so, you should consider to closing the previous span and recreating works.

[zhaoxiaojie0415]

Do you mean to check the current span? How do you verify whether a span is created? I am not sure.

In theory, every retry should create a span, so, you should consider to closing the previous span and recreating works.

I mean to put a flag in ContextManager.getRuntimeContext() when firstly creates span.
I initially thought that regardless of how many retries occur, we should only create a single span to record the start and end times. From the user's perspective, they should only care about this unified timeline. However, I'm not entirely sure if this approach is appropriate.
Even if we create a new span for each retry, using a flag (as discussed earlier) could allow us to disable the first span intentionally, if needed.

[wu-sheng]

I initially thought that regardless of how many retries occur, we should only create a single span to record the start and end times. From the user's perspective, they should only care about this unified timeline. However, I'm not entirely sure if this approach is appropriate.

From the user perspective, the RPC happened multiple times, you should have those. And metrics of requesting should be more than server side traffic, which indicates the retry happens.

[zhaoxiaojie0415]

I initially thought that regardless of how many retries occur, we should only create a single span to record the start and end times. From the user's perspective, they should only care about this unified timeline. However, I'm not entirely sure if this approach is appropriate.

From the user perspective, the RPC happened multiple times, you should have those. And metrics of requesting should be more than server side traffic, which indicates the retry happens.

Okay, also, is it feasible to save a flag in the runtime context to track whether a span has already been created?

[wu-sheng]

Usually no, unless there is no other proper way.
We should try to add an interceptor for the method about processing when error occurred or triggering retry. Then close the previous span with error status.