[DSM] Fix missing produce-side checkpoints for Azure Service Bus#8433
Merged
robcarlan-datadog merged 17 commits intoMay 15, 2026
Conversation
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8433) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8433) - mean (73ms) : 70, 77
master - mean (75ms) : 70, 80
section Bailout
This PR (8433) - mean (80ms) : 76, 83
master - mean (78ms) : 75, 81
section CallTarget+Inlining+NGEN
This PR (8433) - mean (1,130ms) : 1079, 1182
master - mean (1,128ms) : 1084, 1172
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8433) - mean (117ms) : 112, 123
master - mean (115ms) : 108, 121
section Bailout
This PR (8433) - mean (116ms) : 112, 119
master - mean (115ms) : 112, 119
section CallTarget+Inlining+NGEN
This PR (8433) - mean (813ms) : 785, 841
master - mean (813ms) : 780, 846
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8433) - mean (102ms) : 96, 109
master - mean (101ms) : 97, 104
section Bailout
This PR (8433) - mean (106ms) : 100, 111
master - mean (101ms) : 99, 103
section CallTarget+Inlining+NGEN
This PR (8433) - mean (943ms) : 908, 979
master - mean (946ms) : 903, 990
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8433) - mean (100ms) : 97, 103
master - mean (104ms) : 98, 110
section Bailout
This PR (8433) - mean (103ms) : 98, 107
master - mean (101ms) : 98, 105
section CallTarget+Inlining+NGEN
This PR (8433) - mean (841ms) : 781, 900
master - mean (840ms) : 784, 895
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8433) - mean (209ms) : 196, 223
master - mean (211ms) : 190, 231
section Bailout
This PR (8433) - mean (213ms) : 201, 224
master - mean (214ms) : 194, 234
section CallTarget+Inlining+NGEN
This PR (8433) - mean (1,289ms) : 1230, 1349
master - mean (1,318ms) : 1228, 1407
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8433) - mean (304ms) : 272, 335
master - mean (304ms) : 270, 338
section Bailout
This PR (8433) - mean (304ms) : 275, 332
master - mean (301ms) : 277, 325
section CallTarget+Inlining+NGEN
This PR (8433) - mean (1,018ms) : 983, 1053
master - mean (1,018ms) : 976, 1060
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8433) - mean (289ms) : 270, 308
master - mean (299ms) : 274, 325
section Bailout
This PR (8433) - mean (291ms) : 271, 311
master - mean (301ms) : 275, 326
section CallTarget+Inlining+NGEN
This PR (8433) - mean (1,184ms) : 1133, 1236
master - mean (1,213ms) : 1134, 1293
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8433) - mean (293ms) : 273, 314
master - mean (300ms) : 273, 326
section Bailout
This PR (8433) - mean (296ms) : 275, 317
master - mean (303ms) : 271, 335
section CallTarget+Inlining+NGEN
This PR (8433) - mean (1,108ms) : 1000, 1217
master - mean (1,122ms) : 994, 1251
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
BenchmarksBenchmark execution time: 2026-05-08 20:04:00 Comparing candidate commit d9a1cc9 in PR branch Some scenarios are present only in baseline or only in candidate runs. If you didn't create or remove some scenarios in your branch, this maybe a sign of crashed benchmarks 💥💥💥 Scenarios present only in baseline:
Found 4 performance improvements and 1 performance regressions! Performance is the same for 48 metrics, 19 unstable metrics, 84 known flaky benchmarks, 42 flaky benchmarks without significant changes.
|
robcarlan-datadog
force-pushed
the
rob/fix-asb-dsm-produce-checkpoint-v2
branch
from
April 9, 2026 17:47
8d5176f to
1e5a8a4
Compare
robcarlan-datadog
changed the title
robcarlan-datadog
marked this pull request as ready for review
April 29, 2026 16:49
robcarlan-datadog
requested review from
anna-git
and removed request for
a team
April 29, 2026 16:49
Contributor
😢 Does this fix cover this overload? ServiceBusSenderSendMessageBatchAsyncIntegration.cs Was it even supported before? |
Collaborator
|
@codex review |
bouwkast
reviewed
Apr 30, 2026
![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: 362c9898a2
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Author
I don't think we ever supported this for DSM. I can add support as a separate PR (Mostly because I would like to try and get this one into the next release 🤞 ) |
….18.x The DSM produce checkpoint for ServiceBusSender.Send relied on a 3-step Activity chain through InstrumentMessage/Message Activity. SDK >= 7.18.x no longer calls InstrumentMessage, so the checkpoint never fires. Move the produce checkpoint directly into the calltarget integrations (SendMessagesAsync, ScheduleMessagesAsync) which have access to the sender instance and messages. Add a guard flag so the old Activity handler doesn't double-count on older SDK versions. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…alltargets The previous approach kept the Activity handler's produce checkpoint as a fallback for older SDK versions where the "Message" Activity still fires, guarding against double-counting with a ProduceCheckpointSetByCalltarget AsyncLocal flag. The guard is unnecessary: the Activity-based path was already dead code because InstrumentMessageIntegration.OnMethodBegin injects traceparent before InstrumentMessage runs, causing InstrumentMessage to skip creating the Activity. Remove the guard flag and the produce checkpoint block from AzureServiceBusActivityHandler entirely. DSM produce checkpoints are now set exclusively in ServiceBusSenderSendMessagesAsyncIntegration and ServiceBusSenderScheduleMessagesAsyncIntegration, which have direct access to messages for all SDK versions >= 7.14.0. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…n calltargets Delete SendServiceBusMessagesIntegration (hooked CreateDiagnosticScope solely to build the applicationProperties→message weak-table mapping). Strip the DSM block from InstrumentMessageIntegration.OnMethodBegin and its OnMethodEnd (whose only job was clearing the ActiveMessageProperties AsyncLocal). The InjectContext call for W3C trace propagation is kept. Remove ActiveMessageProperties, ApplicationPropertiesToMessageMap, SetMessage, and TryGetMessage from AzureServiceBusCommon — all were only needed to thread message data through to AzureServiceBusActivityHandler.ActivityStopped, which no longer contains any DSM logic. Remove the SetMessage call from SendServiceBusMessageBatchIntegration; the rest of that file (span links) is unaffected. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
… profiler compatibility The native profiler has this type baked into its instrumentation definitions for CreateDiagnosticScope. Deleting the managed class causes a TypeLoadException at runtime. The stub satisfies the native side while doing nothing. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
robcarlan-datadog
force-pushed
the
rob/fix-asb-dsm-produce-checkpoint-v2
branch
from
May 1, 2026 16:26
362c989 to
8c43325
Compare
Fixes double-enumeration of lazy IEnumerable<ServiceBusMessage> arguments: the prior per-sender loops iterated messages once for DSM, then the SDK iterated them again on its own enumeration, discarding the injected context for generator sequences. InstrumentMessage is called by the SDK per-message for all send paths (SendMessageAsync, SendMessagesAsync, SendMessagesAsync(batch), ScheduleMessagesAsync), so injecting DSM there is both correct and exhaustive. An idempotency guard on `dd-pathway-ctx-base64` prevents double-injection when TryAddMessage (batch-with-links) already ran. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
robcarlan-datadog
force-pushed
the
rob/fix-asb-dsm-produce-checkpoint-v2
branch
from
May 1, 2026 16:30
8c43325 to
0f204af
Compare
…led sends InstrumentMessage (MessagingClientDiagnostics) has ref string parameters which the CallTarget rewriter cannot instrument — OnMethodBegin was silently skipped. Move DSM injection to hooks that actually fire: - SendServiceBusMessagesIntegration (CreateDiagnosticScope) for single/IEnumerable - ServiceBusSenderScheduleMessagesAsyncIntegration for scheduled sends Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…CacheKey gained IsConsume param Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…yncIntegration Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
- Correct batch-without-links comment to reference CreateDiagnosticScope hook - Use ITransportSender (not IServiceBusSender) in SendServiceBusMessagesIntegration since only EntityPath is needed Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
… DSM block Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
bouwkast
reviewed
May 1, 2026
bouwkast
left a comment
bouwkast
left a comment
Collaborator
There was a problem hiding this comment.
I think we've changed logic here, do we not have any tests at all for
…tion ScheduleMessagesAsync internally calls CreateDiagnosticScope(IReadOnlyCollection<ServiceBusMessage>, ...) which is already hooked by SendServiceBusMessagesIntegration. Emitting a second checkpoint here causes the span to carry P2 while injected messages carry P1, breaking pathway continuity. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
When AzureServiceBusBatchLinksEnabled=false no per-message span exists, so DSM was silently skipped for SendMessagesAsync(ServiceBusMessageBatch). Now calls SetCheckpoint directly using the active scope's PathwayContext as parent and injects the result into each message's ApplicationProperties. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
bouwkast
reviewed
May 7, 2026
bouwkast
left a comment
bouwkast
left a comment
Collaborator
There was a problem hiding this comment.
I have one set of questions for some of the code. I'm somewhat struggling with wrapping my head around what exactly is expected/going on in tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/Azure/ServiceBus/SendServiceBusMessagesIntegration.cs
…d path Previously SendServiceBusMessagesIntegration set a single checkpoint per SendMessagesAsync(IEnumerable<ServiceBusMessage>) call with payloadSizeBytes=0, undercounting throughput and recording one StatsPoint regardless of batch size. Now matches the behavior of SendServiceBusMessageBatchIntegration: one checkpoint per message with computed message size. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
bouwkast
approved these changes
May 8, 2026
bouwkast
left a comment
bouwkast
left a comment
Collaborator
There was a problem hiding this comment.
Thank you looks good to me now!
robcarlan-datadog
enabled auto-merge (squash)
May 8, 2026 16:31
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
4 participants
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.
DSM produce checkpoints were missing for most Azure Service Bus send paths.
Fix: Move DSM injection to hooks that actually fire:
SendServiceBusMessagesIntegration(CreateDiagnosticScope) — coversSendMessageAsyncandSendMessagesAsync(IEnumerable)ServiceBusSenderScheduleMessagesAsyncIntegration— coversScheduleMessagesAsyncSendServiceBusMessageBatchIntegration(TryAddMessage) — covers batch-with-links (pre-existing, unchanged)Also removes the dead 3-step Activity chain (
ActiveMessagePropertiesAsyncLocal +ApplicationPropertiesToMessageMapweak table +ActivityStoppedDSM block) that was never reachable.Tested the following in a DSM test app:
SendMessageAsync(ServiceBusMessage)— single message sendSendMessagesAsync(IEnumerable<ServiceBusMessage>)— collection send (including lazy LINQ sequences)SendMessagesAsync(ServiceBusMessageBatch)— pre-built batch viaTryAddMessageScheduleMessagesAsync(IEnumerable<ServiceBusMessage>, DateTimeOffset)— scheduled delivery