[SymDB] Stream SymDB payload#8691
Merged
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
BenchmarksBenchmark execution time: 2026-05-28 10:23:53 Comparing candidate commit a87a646 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 71 metrics, 0 unstable metrics, 60 known flaky benchmarks, 66 flaky benchmarks without significant changes.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8691) 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 (8691) - mean (73ms) : 70, 75
master - mean (73ms) : 69, 77
section Bailout
This PR (8691) - mean (77ms) : 75, 79
master - mean (77ms) : 75, 79
section CallTarget+Inlining+NGEN
This PR (8691) - mean (1,111ms) : 1058, 1164
master - mean (1,105ms) : 1049, 1162
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 (8691) - mean (113ms) : 110, 117
master - mean (114ms) : 110, 118
section Bailout
This PR (8691) - mean (119ms) : 113, 124
master - mean (116ms) : 113, 119
section CallTarget+Inlining+NGEN
This PR (8691) - mean (794ms) : 771, 816
master - mean (797ms) : 774, 820
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8691) - mean (104ms) : 98, 110
master - mean (103ms) : 98, 108
section Bailout
This PR (8691) - mean (103ms) : 99, 107
master - mean (104ms) : 99, 110
section CallTarget+Inlining+NGEN
This PR (8691) - mean (957ms) : 921, 994
master - mean (952ms) : 920, 985
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8691) - mean (103ms) : 96, 110
master - mean (99ms) : 96, 102
section Bailout
This PR (8691) - mean (104ms) : 98, 110
master - mean (103ms) : 99, 108
section CallTarget+Inlining+NGEN
This PR (8691) - mean (826ms) : 790, 863
master - mean (825ms) : 792, 859
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 (8691) - mean (198ms) : 192, 204
master - mean (198ms) : 193, 203
section Bailout
This PR (8691) - mean (201ms) : 197, 205
master - mean (202ms) : 199, 205
section CallTarget+Inlining+NGEN
This PR (8691) - mean (1,198ms) : 1148, 1247
master - mean (1,192ms) : 1153, 1230
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 (8691) - mean (284ms) : 275, 293
master - mean (286ms) : 282, 291
section Bailout
This PR (8691) - mean (286ms) : 278, 294
master - mean (288ms) : 282, 294
section CallTarget+Inlining+NGEN
This PR (8691) - mean (959ms) : 940, 977
master - mean (956ms) : 938, 974
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8691) - mean (278ms) : 271, 285
master - mean (278ms) : 273, 283
section Bailout
This PR (8691) - mean (278ms) : 274, 282
master - mean (277ms) : 270, 285
section CallTarget+Inlining+NGEN
This PR (8691) - mean (1,157ms) : 1130, 1185
master - mean (1,159ms) : 1129, 1189
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8691) - mean (277ms) : 270, 283
master - mean (277ms) : 271, 282
section Bailout
This PR (8691) - mean (277ms) : 273, 281
master - mean (276ms) : 269, 284
section CallTarget+Inlining+NGEN
This PR (8691) - mean (1,035ms) : 992, 1078
master - mean (1,035ms) : 991, 1078
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates SymDB symbol uploads to stream payloads directly into the request body (including streaming gzip output) to reduce intermediate allocations/copies, while preserving backend-sensitive multipart behavior via new wire-format tests.
Changes:
- Add a streaming
ISymbolUploadApi.SendBatchAsync(Func<Stream, Task>, ...)overload and switchSymbolsUploaderto streamStringBuildercontent as UTF-8 instead of materializing a full string/byte[]. - Rework
SymbolUploadApimultipart writing to manually stream thefilepart, count the bytes written, then emit theeventpart with the computedattachmentSize. - Add multipart wire-format tests for compressed and uncompressed SymDB uploads (part order, headers, decompression, and event metadata validation).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tracer/test/Datadog.Trace.Tests/Debugger/SymbolsTests/SymbolUploaderTest.cs | Extends mock API to accept the new streaming upload overload. |
| tracer/test/Datadog.Trace.Tests/Debugger/SymbolsTests/SymbolUploadApiTests.cs | Adds multipart wire-format tests for streamed SymDB uploads (gzip and non-gzip). |
| tracer/src/Datadog.Trace/Debugger/Upload/SymbolUploadApi.cs | Implements streamed multipart upload (manual multipart writing + byte counting for attachmentSize). |
| tracer/src/Datadog.Trace/Debugger/Upload/ISymbolUploadApi.cs | Adds streaming upload overload to the SymDB upload API contract. |
| tracer/src/Datadog.Trace/Debugger/Symbols/SymbolsUploader.cs | Switches symbol payload sending to a streaming writer to avoid large allocations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: c13071ad34
ℹ️ 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".
jpbempel
approved these changes
May 27, 2026
dudikeleti
force-pushed
the
dudik/symdb/stream-upload
branch
from
May 27, 2026 09:35
c13071a to
c32c7f7
Compare
dudikeleti
enabled auto-merge (squash)
May 27, 2026 09:36
andrewlock
reviewed
May 27, 2026
Comment on lines
+32
to
+37
| private static readonly byte[] InitialBoundaryBytes = EncodingHelpers.Utf8NoBom.GetBytes("--" + DatadogHttpValues.Boundary + DatadogHttpValues.CrLf); | ||
| private static readonly byte[] BoundaryBytes = EncodingHelpers.Utf8NoBom.GetBytes(DatadogHttpValues.CrLf + "--" + DatadogHttpValues.Boundary + DatadogHttpValues.CrLf); | ||
| private static readonly byte[] FinalBoundaryBytes = EncodingHelpers.Utf8NoBom.GetBytes(DatadogHttpValues.CrLf + "--" + DatadogHttpValues.Boundary + "--" + DatadogHttpValues.CrLf); | ||
| private static readonly byte[] FileJsonHeaderBytes = EncodingHelpers.Utf8NoBom.GetBytes("Content-Type: " + MimeTypes.Json + DatadogHttpValues.CrLf + "Content-Disposition: form-data; name=\"file\"; filename=\"file.json\"" + DatadogHttpValues.CrLf + DatadogHttpValues.CrLf); | ||
| private static readonly byte[] FileGzipHeaderBytes = EncodingHelpers.Utf8NoBom.GetBytes("Content-Type: " + MimeTypes.Gzip + DatadogHttpValues.CrLf + "Content-Disposition: form-data; name=\"file\"; filename=\"file.gz\"" + DatadogHttpValues.CrLf + DatadogHttpValues.CrLf); | ||
| private static readonly byte[] EventHeaderBytes = EncodingHelpers.Utf8NoBom.GetBytes("Content-Type: " + MimeTypes.Json + DatadogHttpValues.CrLf + "Content-Disposition: form-data; name=\"event\"; filename=\"event.json\"" + DatadogHttpValues.CrLf + DatadogHttpValues.CrLf); |
Member
There was a problem hiding this comment.
We should use ReadonlySpan<byte> for these constants instead, to avoid allocation and to reduce startup overhead 🙂
e.g. #8487
Contributor
Author
There was a problem hiding this comment.
I had your pr in mind 🙂 I wasn’t sure at first if it was better here becuase implementation tradeoff (It adds complexity), but definitely worth to reduce startup overhead
Member
There was a problem hiding this comment.
:/ it looks like you're copying these back into a buffer in that commit? That seems to defeat the purpose of using ReadonlySpan<byte> here 🤔
Contributor
Author
There was a problem hiding this comment.
yes, it's a startup time allocation vs rutime overhead :( that why I wrote that "I wasn’t sure" but maybe startup time is more important here, the new overhead is one shared array per upload. Anyway, I plan to make some changes so I might avoid this new copy later.
Contributor
Author
There was a problem hiding this comment.
I changed this to avoid the per-upload copy. We have now the bytes arrays again but we avoid allocating them till the first symdb upload.
Comment on lines
+63
to
+64
| _discoveryCallback = OnDiscoveryServiceChanged; | ||
| discoveryService.SubscribeToChanges(_discoveryCallback); |
Member
There was a problem hiding this comment.
This is just an unnecessary field I think? You can still unsubscribe later with the method group?
Suggested change
| _discoveryCallback = OnDiscoveryServiceChanged; | |
| discoveryService.SubscribeToChanges(_discoveryCallback); | |
| discoveryService.SubscribeToChanges(OnDiscoveryServiceChanged); |
Member
There was a problem hiding this comment.
Unless you can't, in which case this is a bug we have elsewhere that we should fix 😅
Contributor
Author
There was a problem hiding this comment.
We can do that, I kept this field only because RemoveSubscription(), but i don't have to
Member
There was a problem hiding this comment.
We can do that, I kept this field only because RemoveSubscription(), but i don't have to
But can't you do this?
discoveryService.RemoveSubscription(OnDiscoveryServiceChanged);That's what I meant by this: 😄
Unless you can't, in which case this is a bug we have elsewhere that we should fix 😅
Contributor
Author
There was a problem hiding this comment.
But can't you do this?
yes if you want😅
react with 👍🏼 or 👎🏼 (the current code doesnt bother me, but I'm fine also with removing it. its one field vs allocate the callback when we want to unsubscribe)
dudikeleti
disabled auto-merge
May 27, 2026 22:41
Stops the SymDB upload API from keeping its discovery callback after the SymbolDB endpoint has been found, while preserving thread-safe endpoint publication through the shared debugger upload API base.
…i down to the single overload
dudikeleti
force-pushed
the
dudik/symdb/stream-upload
branch
from
May 28, 2026 09:30
5e5b21b to
a87a646
Compare
dudikeleti
enabled auto-merge (squash)
May 28, 2026 09:41
andrewlock
approved these changes
May 28, 2026
andrewlock
left a comment
andrewlock
left a comment
Member
There was a problem hiding this comment.
Thanks, it might be worth double checking your benchmarks, just before merge, given there were quite a few changes afterwards! 🙂
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.
Summary of changes
ISymbolUploadApioverload for SymDB payload writers.filepart can be streamed and theeventmetadata can still include the final attachment size.Reason for change
Implementation details
SymbolsUploaderkeeps the existing batch-building flow, but writesStringBuilderchunks to aStreamWriterasynchronously instead of materializing a final string and byte array.SymbolUploadApistreams thefilepart first, counts the actual bytes written through a counting stream, then writes theeventpart with the computedattachmentSize.Dispose; the .NET Framework SharpZipLib path remains synchronous.Test coverage
SymbolUploaderTest|SymbolUploadApiTestsOther details
Local BenchmarkDotNet results for a 1 MB SymDB batch on
net8.0:UploadClassesmean48.186 ms41.356 ms-14.2%UploadClassesallocated25,800.36 KB16,022.55 KB-37.9%SendCompressedMultipartmean1.922 ms46.53 us-97.6%SendCompressedMultipartallocated77.95 KB4.52 KB-94.2%