Spans are lost if the same client is used for multiple requests concurrently

[snake-scaly]

We've noticed recently in a high-load project I'm involved in that not all DynamoDB spans are reaching our APM. I have traced this issue down to the Activity field in the AWSTracingPipelineHandler.

When the same AmazonDynamoDBClient is used concurrently to perform multiple async requests to Dynamo, this field gets overwritten by the most recent request. The Activities for the earlier requests become orphaned. The orphaned activities are never stopped and therefore never reach the collector. We confirmed that the same thing happens with concurrent requests to AmazonSQSClient, and I'm pretty sure to any other client with a RuntimePipeline. Note that AWS clients are designed to be able to handle concurrent requests, and work as expected without the OpenTelemetry instrumentation.

The fix is to make the Activity field AsyncLocal. I did test this fix by including the latest Main of this repository into our project instead of the NuGet package and fixing it locally. I can confirm that it solves the problem of disappearing spans.

Below is a patch with my fix. I can create a pull request if you prefer it that way.

diff --git a/src/OpenTelemetry.Instrumentation.AWS/Implementation/AWSTracingPipelineHandler.cs b/src/OpenTelemetry.Instrumentation.AWS/Implementation/AWSTracingPipelineHandler.cs
index 979d414f31..2e82cb1c89 100644
--- a/src/OpenTelemetry.Instrumentation.AWS/Implementation/AWSTracingPipelineHandler.cs
+++ b/src/OpenTelemetry.Instrumentation.AWS/Implementation/AWSTracingPipelineHandler.cs
@@ -24,6 +24,8 @@ internal sealed class AWSTracingPipelineHandler : PipelineHandler
 
     private static readonly ActivitySource AWSSDKActivitySource = new(ActivitySourceName, typeof(AWSTracingPipelineHandler).Assembly.GetPackageVersion());
 
+    private readonly AsyncLocal<Activity?> activity = new();
+
     private readonly AWSClientInstrumentationOptions options;
 
     public AWSTracingPipelineHandler(AWSClientInstrumentationOptions options)
@@ -31,11 +33,11 @@ internal sealed class AWSTracingPipelineHandler : PipelineHandler
         this.options = options;
     }
 
-    public Activity? Activity { get; private set; }
+    public Activity? Activity => activity.Value;
 
     public override void InvokeSync(IExecutionContext executionContext)
     {
-        this.Activity = this.ProcessBeginRequest(executionContext);
+        this.activity.Value = this.ProcessBeginRequest(executionContext);
         try
         {
             base.InvokeSync(executionContext);
@@ -62,7 +64,7 @@ internal sealed class AWSTracingPipelineHandler : PipelineHandler
     {
         T? ret = null;
 
-        this.Activity = this.ProcessBeginRequest(executionContext);
+        this.activity.Value = this.ProcessBeginRequest(executionContext);
         try
         {
             ret = await base.InvokeAsync<T>(executionContext).ConfigureAwait(false);

[vastin]

Hi @snake-scaly, thank you for reporting this issue. Your contribution will be always welcomed and we are happy to help review pull request.

[AsakerMohd]

Hey @snake-scaly, this has been fixed in the latest version of the Opentelemetry.Instrumentation.AWS here. We are currently working on bringing those changes to our package.

[snake-scaly]

Thank you, this nideed should fix the problem. Should I close my PR then?

[AsakerMohd]

Yes pls. Thanks for bringing it up!