Codebase consistency: call scope.close() before span.finish()#11922
Draft
mcculls wants to merge 1 commit into
Draft
Codebase consistency: call scope.close() before span.finish()#11922mcculls wants to merge 1 commit into
mcculls wants to merge 1 commit into
Conversation
mcculls
added
inst: others
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR standardizes scope/span lifecycle ordering across a broad set of instrumentations by closing the active AgentScope before finishing the associated AgentSpan, improving consistency and reducing the chance of a finished span remaining active on the thread.
Changes:
- Reorders cleanup logic to call
scope.close()beforespan.finish()(orfinishWithEndToEnd()) across many instrumentations. - Simplifies several exit/cleanup blocks by removing
try/finallywhere the scope is now closed earlier in the method. - Updates one RxJava test helper to follow the same close-then-finish ordering.
Reviewed changes
Copilot reviewed 61 out of 61 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| dd-java-agent/instrumentation/undertow/undertow-2.0/src/main/java/datadog/trace/instrumentation/undertow/HttpRequestParserInstrumentation.java | Reorder scope.close() before span.finish() in request-parse cleanup. |
| dd-java-agent/instrumentation/twilio-0.0.1/src/main/java/datadog/trace/instrumentation/twilio/TwilioAsyncInstrumentation.java | Close scope earlier and finish span after async/error handling setup. |
| dd-java-agent/instrumentation/tibco-businessworks/tibco-businessworks-6.5/src/main/java/datadog/trace/instrumentation/tibcobw6/BehaviorInstrumentation.java | Close scope before finishing/stashing span for later completion. |
| dd-java-agent/instrumentation/synapse-3.0/src/main/java/datadog/trace/instrumentation/synapse3/SynapseServerWorkerInstrumentation.java | Close scope up-front in response handling and finish span conditionally. |
| dd-java-agent/instrumentation/synapse-3.0/src/main/java/datadog/trace/instrumentation/synapse3/SynapseServerInstrumentation.java | Close scope up-front in response handling and finish span conditionally. |
| dd-java-agent/instrumentation/synapse-3.0/src/main/java/datadog/trace/instrumentation/synapse3/SynapseClientInstrumentation.java | Close scope up-front in response handling and finish span conditionally. |
| dd-java-agent/instrumentation/servlet/javax-servlet/javax-servlet-3.0/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Advice.java | Close scope earlier in stopSpan advice (before response tagging). |
| dd-java-agent/instrumentation/rxjava/rxjava-3.0/src/test/java/testdog/trace/instrumentation/rxjava3/RxJava3Test.java | Test helper aligns ordering: scope.close() before span.finish(). |
| dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsInstrumentation.java | Close scope before span finish / async suspend checks. |
| dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/DefaultRequestContextInstrumentation.java | Reorder to close scope before finishing span. |
| dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAnnotationsInstrumentation.java | Close scope before span finish / async suspend checks. |
| dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/DefaultRequestContextInstrumentation.java | Reorder to close scope before finishing span. |
| dd-java-agent/instrumentation/rediscala-1.8/src/main/java/datadog/trace/instrumentation/rediscala/RediscalaInstrumentation.java | Close scope earlier; span finished inline or via completion handler. |
| dd-java-agent/instrumentation/rabbitmq-amqp-2.7/src/main/java/datadog/trace/instrumentation/rabbitmq/amqp/RabbitDecorator.java | Close receiving scope before finishing span (including E2E finish). |
| dd-java-agent/instrumentation/quartz-2.0/src/main/java/datadog/trace/instrumentation/quartz/QuartzSchedulingInstrumentation.java | Reorder to close scope before finishing span. |
| dd-java-agent/instrumentation/play-ws/play-ws-2.1/src/main/java/datadog/trace/instrumentation/playws21/PlayWSClientInstrumentation.java | Close scope before error tagging/finish; async spans finish elsewhere. |
| dd-java-agent/instrumentation/play-ws/play-ws-2.0/src/main/java/datadog/trace/instrumentation/playws2/PlayWSClientInstrumentation.java | Close scope before error tagging/finish; async spans finish elsewhere. |
| dd-java-agent/instrumentation/play-ws/play-ws-1.0/src/main/java/datadog/trace/instrumentation/playws1/PlayWSClientInstrumentation.java | Close scope before error tagging/finish; async spans finish elsewhere. |
| dd-java-agent/instrumentation/pekko/pekko-http-1.0/src/main/java/datadog/trace/instrumentation/pekkohttp/PekkoHttpSingleRequestInstrumentation.java | Close scope before finishing span / attaching async completion. |
| dd-java-agent/instrumentation/opensearch/opensearch-transport-1.0/src/main/java/datadog/trace/instrumentation/opensearch/OpensearchTransportClientInstrumentation.java | Close scope early; error path finishes span inline. |
| dd-java-agent/instrumentation/opensearch/opensearch-rest-1.0/src/main/java/datadog/trace/instrumentation/opensearch/OpensearchRestClientInstrumentation.java | Close scope early; error path finishes span inline. |
| dd-java-agent/instrumentation/lettuce/lettuce-4.0/src/main/java/datadog/trace/instrumentation/lettuce4/InstrumentationPoints.java | Close scope early; span may finish inline or via async handling. |
| dd-java-agent/instrumentation/kafka/kafka-clients-3.8/src/main/java17/datadog/trace/instrumentation/kafka_clients38/RecordsAdvice.java | Close scope before finishing span after tagging records count. |
| dd-java-agent/instrumentation/kafka/kafka-clients-0.11/src/main/java/datadog/trace/instrumentation/kafka_clients/KafkaConsumerInfoInstrumentation.java | Close scope before finishing span after tagging records count. |
| dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.0/src/main/java/datadog/trace/instrumentation/jetty9/ServerHandleInstrumentation.java | Close scope before conditional finish / async listener path. |
| dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-7.6/src/main/java/datadog/trace/instrumentation/jetty76/ServerHandleInstrumentation.java | Close scope before conditional finish / async listener path. |
| dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-7.0/src/main/java/datadog/trace/instrumentation/jetty70/ServerHandleInstrumentation.java | Close scope before conditional finish / async listener path. |
| dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-10.0/src/main/java11/datadog/trace/instrumentation/jetty10/ServerHandleAdvice.java | Close scope before conditional finish / request cleanup. |
| dd-java-agent/instrumentation/java/java-net/java-net-11.0/src/main/java11/datadog/trace/instrumentation/httpclient/SendAsyncAdvice.java | Close scope before error tagging/finish; async completion keeps span. |
| dd-java-agent/instrumentation/ignite-2.0/src/main/java/datadog/trace/instrumentation/ignite/v2/cache/IgniteCacheAsyncInstrumentation.java | Close scope early; finish span on sync error or via async callback. |
| dd-java-agent/instrumentation/hibernate/hibernate-common/src/main/java/datadog/trace/instrumentation/hibernate/SessionMethodUtils.java | Close scope and clear state before conditional span finish. |
| dd-java-agent/instrumentation/hazelcast/hazelcast-4.0/src/main/java/datadog/trace/instrumentation/hazelcast4/InvocationAdvice.java | Close scope early; sync error finishes span, async finishes later. |
| dd-java-agent/instrumentation/hazelcast/hazelcast-4.0/src/main/java/datadog/trace/instrumentation/hazelcast4/ClientListenerInstrumentation.java | Close scope/reset call depth earlier; finish span in finally. |
| dd-java-agent/instrumentation/hazelcast/hazelcast-3.9/src/main/java/datadog/trace/instrumentation/hazelcast39/ClientInvocationInstrumentation.java | Close scope early; sync error finishes span, async finishes later. |
| dd-java-agent/instrumentation/hazelcast/hazelcast-3.6/src/main/java/datadog/trace/instrumentation/hazelcast36/DistributedObjectInstrumentation.java | Close scope early; sync error finishes span, async finishes later. |
| dd-java-agent/instrumentation/grpc-1.5/src/main/java/datadog/trace/instrumentation/grpc/client/MessagesAvailableInstrumentation.java | Close scope before finishing span in message-availability advice. |
| dd-java-agent/instrumentation/grizzly/grizzly-client-1.9/src/main/java/datadog/trace/instrumentation/grizzly/client/AsyncHttpClientInstrumentation.java | Close scope early; error path finishes span inline. |
| dd-java-agent/instrumentation/grizzly/grizzly-2.0/src/main/java/datadog/trace/instrumentation/grizzly/GrizzlyHttpHandlerInstrumentation.java | Close scope early; error path finishes span inline. |
| dd-java-agent/instrumentation/google-pubsub-1.116/src/main/java/datadog/trace/instrumentation/googlepubsub/PublisherInstrumentation.java | Close scope before finishing span in publisher exit advice. |
| dd-java-agent/instrumentation/google-http-client-1.19/src/main/java/datadog/trace/instrumentation/googlehttpclient/GoogleHttpClientInstrumentation.java | Close scope earlier; perform decoration and finish span after close. |
| dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/Listener.java | Close scope before finishing span in success/failure callbacks. |
| dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java | Close scope before finishing span on throwable path. |
| dd-java-agent/instrumentation/elasticsearch/elasticsearch-transport/elasticsearch-transport-7.3/src/main/java/datadog/trace/instrumentation/elasticsearch7_3/Elasticsearch73TransportClientInstrumentation.java | Close scope early; error path finishes span inline. |
| dd-java-agent/instrumentation/elasticsearch/elasticsearch-transport/elasticsearch-transport-6.0/src/main/java/datadog/trace/instrumentation/elasticsearch6/Elasticsearch6TransportClientInstrumentation.java | Close scope early; error path finishes span inline. |
| dd-java-agent/instrumentation/elasticsearch/elasticsearch-transport/elasticsearch-transport-5.3/src/main/java/datadog/trace/instrumentation/elasticsearch5_3/Elasticsearch53TransportClientInstrumentation.java | Close scope early; error path finishes span inline. |
| dd-java-agent/instrumentation/elasticsearch/elasticsearch-transport/elasticsearch-transport-5.0/src/main/java/datadog/trace/instrumentation/elasticsearch5/Elasticsearch5TransportClientInstrumentation.java | Close scope early; error path finishes span inline. |
| dd-java-agent/instrumentation/elasticsearch/elasticsearch-transport/elasticsearch-transport-2.0/src/main/java/datadog/trace/instrumentation/elasticsearch2/Elasticsearch2TransportClientInstrumentation.java | Close scope early; error path finishes span inline. |
| dd-java-agent/instrumentation/elasticsearch/elasticsearch-rest/elasticsearch-rest-7.0/src/main/java/datadog/trace/instrumentation/elasticsearch7/Elasticsearch7RestClientInstrumentation.java | Close scope early; error path finishes span inline. |
| dd-java-agent/instrumentation/elasticsearch/elasticsearch-rest/elasticsearch-rest-6.4/src/main/java/datadog/trace/instrumentation/elasticsearch6_4/Elasticsearch6RestClientInstrumentation.java | Close scope early; error path finishes span inline. |
| dd-java-agent/instrumentation/elasticsearch/elasticsearch-rest/elasticsearch-rest-5.0/src/main/java/datadog/trace/instrumentation/elasticsearch5/Elasticsearch5RestClientInstrumentation.java | Close scope early; error path finishes span inline. |
| dd-java-agent/instrumentation/datanucleus-4.0.5/src/main/java/datadog/trace/instrumentation/datanucleus/JDOTransactionInstrumentation.java | Reorder to close scope before finishing span. |
| dd-java-agent/instrumentation/datanucleus-4.0.5/src/main/java/datadog/trace/instrumentation/datanucleus/JDOQueryInstrumentation.java | Reorder to close scope before finishing span. |
| dd-java-agent/instrumentation/datanucleus-4.0.5/src/main/java/datadog/trace/instrumentation/datanucleus/ExecutionContextInstrumentation.java | Reorder to close scope before finishing span in multiple end-method advices. |
| dd-java-agent/instrumentation/cucumber-5.4/src/main/java/datadog/trace/instrumentation/cucumber/CucumberStepDecorator.java | Reorder to close scope before finishing step span. |
| dd-java-agent/instrumentation/cics-9.1/src/main/java/datadog/trace/instrumentation/cics/JavaGatewayInterfaceInstrumentation.java | Reorder to close scope before finishing span. |
| dd-java-agent/instrumentation/cics-9.1/src/main/java/datadog/trace/instrumentation/cics/ECIInteractionInstrumentation.java | Close scope before finishing span; avoid repeated scope.span() calls. |
| dd-java-agent/instrumentation/aws-java/aws-java-lambda-handler-1.2/src/main/java/datadog/trace/instrumentation/aws/v1/lambda/LambdaHandlerInstrumentation.java | Close scope earlier; perform final tagging/notifications then finish span. |
| dd-java-agent/instrumentation/armeria/armeria-grpc-0.84/src/main/java/datadog/trace/instrumentation/armeria/grpc/client/ClientCallImplInstrumentation.java | Close scope before on-close decoration and finishing span. |
| dd-java-agent/instrumentation/apache-httpclient/apache-httpclient-5.0/src/main/java/datadog/trace/instrumentation/apachehttpclient5/ApacheHttpAsyncClientInstrumentation.java | Close scope early; error path finishes span inline. |
| dd-java-agent/instrumentation/akka/akka-http/akka-http-10.6/src/main/java11/datadog/trace/instrumentation/akkahttp106/SingleRequestAdvice.java | Close scope before error tagging/finish; async completion handles span. |
| dd-java-agent/instrumentation/akka/akka-http/akka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpSingleRequestInstrumentation.java | Close scope before error tagging/finish; async completion handles span. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| scope.close(); | ||
| CallDepthThreadLocalMap.reset(Twilio.class); // reset call depth count | ||
| if (throwable != null) { | ||
| // There was an synchronous error, |
| @@ -89,24 +89,20 @@ public static void stopSpan( | |||
| return; | |||
| } | |||
| // If we have a scope (i.e. we were the top-level Twilio SDK invocation), | |||
| scope.close(); | ||
| CallDepthThreadLocalMap.reset(IgniteCache.class); // reset call depth count | ||
| if (throwable != null) { | ||
| // There was an synchronous error, |
| @@ -144,24 +140,20 @@ public static void stopSpan( | |||
| return; | |||
| } | |||
| // If we have a scope (i.e. we were the top-level Twilio SDK invocation), | |||
| scope.close(); | ||
| CallDepthThreadLocalMap.reset(IgniteCache.class); // reset call depth count | ||
| if (throwable != null) { | ||
| // There was an synchronous error, |
| scope.close(); | ||
| CallDepthThreadLocalMap.reset(ClientInvocation.class); // reset call depth count | ||
| if (throwable != null) { | ||
| // There was an synchronous error, |
| scope.close(); | ||
| CallDepthThreadLocalMap.reset(ClientInvocation.class); // reset call depth count | ||
| if (throwable != null) { | ||
| // There was an synchronous error, |
| scope.close(); | ||
| CallDepthThreadLocalMap.reset(DistributedObject.class); // reset call depth count | ||
| if (throwable != null) { | ||
| // There was an synchronous error, |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adb2c9408a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @Advice.Return(readOnly = false) final Future<Object> responseFuture) { | ||
|
|
||
| final AgentSpan span = scope.span(); | ||
| scope.close(); |
There was a problem hiding this comment.
For successful Rediscala sends (throwable == null), closing the scope here restores the parent/no context before responseFuture.onComplete(...) registers OnCompleteHandler. That handler explicitly relies on Scala promise propagation and calls activeSpan() in OnCompleteHandler.apply; with the client span no longer active at registration, the callback runs with the wrong span or none, so the Redis command span is not finished and can even finish its parent. Register the callback while the Rediscala span is still active, then close the scope before any synchronous finish path.
Useful? React with 👍 / 👎.
| return; | ||
| } | ||
|
|
||
| scope.close(); |
There was a problem hiding this comment.
For servlet requests with IAST request-end checks enabled, closing the request scope here means the later DECORATE.beforeFinish(scope.context()) calls run after AgentTracer.activeSpan() has been restored to the parent or null. HttpServerDecorator.beforeFinish fires the requestEnded callbacks, and the HSTS/X-Content-Type request-end modules report using AgentTracer.activeSpan(), so missing-header vulnerabilities can be attached to the wrong span or dropped. Keep the servlet scope active until after beforeFinish, then close it before span.finish().
Useful? React with 👍 / 👎.
| return; | ||
| } | ||
|
|
||
| scope.close(); |
There was a problem hiding this comment.
When IAST is enabled for Jetty requests that finish in this advice, closing the scope before DECORATE.beforeFinish(scope.context()) restores the parent/null active span before HttpServerDecorator.beforeFinish fires the requestEnded callbacks. The HSTS/X-Content-Type request-end checks report with AgentTracer.activeSpan(), so direct/non-async Jetty requests can lose or misattribute those vulnerabilities; keep the Jetty scope active until after beforeFinish, then close it before span.finish().
Useful? React with 👍 / 👎.
Contributor
|
🎯 Code Coverage (details) 🔗 Commit SHA: adb2c94 | Docs | Datadog PR Page | Give us feedback! |
Contributor
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
Kafka / producer-benchmarkParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 3 metrics, 0 unstable metrics. See unchanged results
|
Kafka / consumer-benchmarkParameters
See matching parameters
SummaryFound 1 performance improvements and 0 performance regressions! Performance is the same for 2 metrics, 0 unstable metrics.
See unchanged results
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Provide more consistent examples for new developers
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]