perf: multiple improvements backport [v2.10.0]#5019
Open
darccio wants to merge 6 commits into
Open
Conversation
… path (#4999) ## Problem The DSM consume checkpoint runs on every Kafka message. `datastreams.ExtractFromBase64Carrier` only needs one header (`dd-pathway-ctx-base64`), but it reads the carrier via the generic `ForeachKey` contract, which forces the carrier to convert **every** header value `[]byte`→`string` — one heap allocation per header, all but one immediately discarded. On a high-throughput consumer that allocation rate is a material GC driver. ### Proof (allocation profile) Profiling the consume hot path of a high-throughput Kafka consumer, a single line accounted for **61.7% of all allocations** in the extract path: ``` ROUTINE ======== MessageCarrier.ForeachKey 2079588 2376672 (flat, cum) 70.56% of Total . . for _, h := range c.msg.GetHeaders() { 2079588 2376672 if err := handler(h.GetKey(), string(h.GetValue())); err != nil { ``` `string(h.GetValue())` allocates a fresh string per header; the DSM callback keeps only the pathway header and discards the rest. ## What changed - Add an optional `datastreams.TextMapReaderByKey` interface (`Get(key) (string, bool)`). `ExtractFromBase64Carrier` uses it when the carrier implements it, and falls back to `ForeachKey` otherwise. - Implement `Get` on all Kafka carriers — confluent, Shopify & IBM sarama (producer + consumer), segmentio — converting only the matched header's value. - APM trace propagation (which legitimately reads many headers via `ForeachKey`) is untouched. - `Get` matches `ForeachKey`'s last-wins semantics for duplicate headers, and each carrier has a `var _ datastreams.TextMapReaderByKey` compile-time assertion so the fast path can't silently regress. ## Results `ExtractFromBase64Carrier` on the same message (pathway header + 10 others), Apple M3 Max, measured both ways on the identical carrier: | Carrier | Before (`ForeachKey`) | After (fast path) | |---|---|---| | confluent | 384 ns, 872 B, 17 allocs | 205 ns, 360 B, 6 allocs | | Shopify/sarama | 449 ns, 880 B, 27 allocs | 133 ns, 184 B, 5 allocs | | IBM/sarama | 453 ns, 880 B, 27 allocs | 133 ns, 184 B, 5 allocs | | segmentio | 342 ns, 696 B, 16 allocs | 151 ns, 184 B, 5 allocs | Benchmarks are committed (`BenchmarkExtract*` in each carrier package) and reproduce the before/after by hiding `Get` to force the fallback path. ## Notes - Complementary to #4920 (hash-cache fingerprint, edge-tag cache, processtags): that PR reduced allocations on the checkpoint/tag side; this one removes them on the header-extract side. Different files, different allocation sources. - Scope is Kafka carriers only for now. Any carrier can opt in by implementing `TextMapReaderByKey`. - This does not remove the header-slice materialization inside each carrier's `GetHeaders()`; that would require a by-key accessor on the carriers' `Message` interfaces (a larger, cross-module change) and is deliberately left as a follow-up. ## End-to-end benchmark (Kafka consumer) Reproduced the original slow-consume scenario in a standalone app: a DSM-only consume path (plain confluent consumer + manual extract + checkpoint, no per-message APM span, matching the reporting service). Local Kafka, 20 headers per message, consumer CPU-bound at ~80k msg/s, 30s CPU profiles, baseline (`origin/main`) vs this branch under identical load. DSM per-message work (`trackConsume` = extract + checkpoint), from `go tool pprof`: | | Baseline | This PR | |---|---|---| | `trackConsume` (whole DSM op) | 2.41s | 1.64s (**-32%**) | | `ExtractFromBase64Carrier` | 1.74s | 0.98s (**-44%**) | In the flame graph the per-header `runtime.slicebytetostring` block (the string copies) disappears after the fix; only the matched pathway header is converted. (pprof focus-view total for `trackConsume`: 2.41s -> 1.64s.) Co-authored-by: jj.botha <jj.botha@datadoghq.com>
…extraction (#5005) ### What does this PR do? Two low-risk, high-confidence allocation reductions on the request path: - **`ddtrace/tracer/textmap.go`**: propagation extractors ran `strings.ToLower(k)` on every incoming header before comparing it against known header names, allocating a throwaway lowercased string whenever the carrier used canonical case (e.g. Go's `http.Header`, as used by `HTTPHeadersCarrier`). Switches `propagator`, `propagatorB3`, `propagatorB3SingleHeader`, `propagatorW3c`, and `propagatorBaggage` extraction to `strings.EqualFold`-based dispatch instead, and adds a `cutPrefixFold` helper for the OT baggage-prefix checks so only the matched prefix is case-folded, not the whole header. Also makes the `pendingBaggage` map in `extractIncomingSpanContext` lazily allocated instead of eagerly created on every extraction call. - **`instrumentation/httptrace/httptrace.go`**: pre-sizes the per-request span tag map built in `StartRequestSpan` (it always ends up with several entries, so growing it from empty was wasted work). ### Motivation Profiling of a small, high-throughput HTTP+Redis+DB edge service showed dd-trace-go contributing a disproportionate share of allocation rate relative to CPU. These two sites were identified as high-confidence, low-risk wins on the extraction/span-start hot path. Verified with a throwaway benchmark using realistic canonical-case HTTP headers (not exercised by the existing checked-in benchmarks, which use pre-lowercased map keys and therefore hit `strings.ToLower`'s no-alloc fast path): Datadog extraction dropped from 5→4 allocs/op, W3C from 24→22 allocs/op. Caught during implementation: naively swapping `ToLower`→`EqualFold` broke `TestPropagationDefaults`/`TestPropagationDefaultIncludesBaggage`, because the old blanket lowercasing also normalized the *suffix* of OT baggage-prefixed keys (`ot-baggage-X` → baggage key `x`), which callers rely on for map-key matching. Fixed by explicitly lowercasing just that suffix after the case-insensitive prefix match, preserving exact prior behavior. ### Reviewer's Checklist - [x] Changed code has unit tests for its functionality at or near 100% coverage. - [ ] [System-Tests](https://github.com/DataDog/system-tests/) covering this feature have been added and enabled with the va.b.c-dev version tag. - [x] There is a benchmark for any new code, or changes to existing code. - [ ] If this interacts with the agent in a new way, a system test has been added. - [x] New code is free of linting errors. You can check this by running `make lint` locally. - [x] New code doesn't break existing tests. You can check this by running `make test` locally. - [ ] Add an appropriate team label so this PR gets put in the right place for the release notes. - [x] All generated files are up to date. You can check this by running `make generate` locally. - [ ] Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild. Make sure all nested modules are up to date by running `make fix-modules` locally. Co-authored-by: kemal.akkoyun <kemal.akkoyun@datadoghq.com>
…5010) ### What does this PR do? Drop the ToLower and add isValidIDCaseInsensitive for the W3C trace/span ID checks, preserving the existing lenient (case-insensitive) accept behavior while removing a redundant scan/allocation on every extract. isValidID itself is untouched, so the Datadog propagator's _dd.p.tid check keeps its current (stricter) behavior. ### Motivation parseTraceparent lowercased the entire traceparent header on every W3C extract, even though every downstream consumer (ParseUint/ParseInt base-16, SetUpperFromHex, cacheHex) is already case-insensitive or canonicalizes to lowercase on write. The only case-sensitive check was isValidID, which rejected uppercase hex. ### Reviewer's Checklist - [ ] Changed code has unit tests for its functionality at or near 100% coverage. - [ ] [System-Tests](https://github.com/DataDog/system-tests/) covering this feature have been added and enabled with the va.b.c-dev version tag. - [ ] There is a benchmark for any new code, or changes to existing code. - [ ] If this interacts with the agent in a new way, a system test has been added. - [ ] New code is free of linting errors. You can check this by running `make lint` locally. - [ ] New code doesn't break existing tests. You can check this by running `make test` locally. - [ ] Add an appropriate team label so this PR gets put in the right place for the release notes. - [ ] All generated files are up to date. You can check this by running `make generate` locally. - [ ] Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild. Make sure all nested modules are up to date by running `make fix-modules` locally. Unsure? Have a question? Request a review! Co-authored-by: dario.castane <dario.castane@datadoghq.com>
github-actions
Bot
added
the
apm:ecosystem
### What does this PR do? Fixes a startup race in `os.TestCmdi` (orchestrion integration suite). `Setup` allocated a port with `net.FreePort(t)` and started the HTTP server asynchronously with `go tc.Server.ListenAndServe()`, but nothing guaranteed the socket was bound before `Run` issued its `http.Get`. On a busy CI host this occasionally raced, producing `connection refused`. The fix switches to `net.FreeListener(t)` + `Server.Serve(ln)`: the listener is bound synchronously in `Setup` before the goroutine starts, so the socket is guaranteed ready when `Run` connects. This is the exact same pattern already used by the sibling test case `os/lfi.go` in the same package — `cmdi.go` was the lone straggler still using the racy `ListenAndServe()` form. ### Motivation CI flake: ``` Get "http://127.0.0.1:50734/?command=/usr/bin/touch%20/tmp/passwd": dial tcp 127.0.0.1:50734: connect: connection refused ``` ### Verification - `go build` / `go vet` clean on the package - `orchestrion go test -run TestCmdi -count=5 -v .` — 5/5 pass - `orchestrion go test -race .` — full package passes under race detector - `golangci-lint run --disable=gocritic ./os/...` — 0 issues ### Reviewer's Checklist - [x] Changed code has unit tests for its functionality at or near 100% coverage. - [ ] [System-Tests](https://github.com/DataDog/system-tests/) covering this feature have been added and enabled with the va.b.c-dev version tag. - [ ] There is a benchmark for any new code, or changes to existing code. - [ ] If this interacts with the agent in a new way, a system test has been added. - [x] New code is free of linting errors. You can check this by running `make lint` locally. - [x] New code doesn't break existing tests. You can check this by running `make test` locally. - [ ] Add an appropriate team label so this PR gets put in the right place for the release notes. - [x] All generated files are up to date. You can check this by running `make generate` locally. - [ ] Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild. Make sure all nested modules are up to date by running `make fix-modules` locally. Co-authored-by: dario.castane <dario.castane@datadoghq.com>
Contributor
Config Audit |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 1698dcb | Docs | Datadog PR Page | Give us feedback! |
…es by replacing FreePort+ListenAndServe with a pre-bound FreeListener+Serve and adding a TestCasePreBootstrap interface so os/lfi.go can configure AppSec env vars before the tracer starts
…eck to 1.26.5 (#5002) ### What does this PR do? Two unrelated fixes for CI checks that are currently red on `main` and every open PR: 1. Bumps the transitive dependency `github.com/containerd/ttrpc` from `v1.2.8` to `v1.2.9` in the generated module files. This regenerates `orchestrion/all/go.mod`, `orchestrion/all/go.sum`, and `internal/orchestrion/_integration/go.sum` via `./scripts/generate.sh` and `./scripts/fix_modules.sh`. 2. Bumps the Go version pinned in the `govulncheck-tests` job from `1.26.4` to `1.26.5` in `.github/workflows/govulncheck.yml`. ### Motivation Both checks fail on `main` independently of any code change: - **Generated files drift:** Upstream published `containerd/ttrpc v1.2.9`, and regenerating the orchestrion modules now resolves to that version, leaving the checked-in `go.mod`/`go.sum` files out of date. This breaks the `generate`, `Verify generated files are up-to-date`, and `check-modules` checks. - **govulncheck:** The `govulncheck-tests` job pinned Go `1.26.4`, which govulncheck flags for `GO-2026-5856` (a `crypto/tls` Encrypted Client Hello privacy leak). The fix shipped in Go `1.26.5`, released 2026-07-07. ### Reviewer's Checklist - [ ] Changed code has unit tests for its functionality at or near 100% coverage. - [ ] [System-Tests](https://github.com/DataDog/system-tests/) covering this feature have been added and enabled with the va.b.c-dev version tag. - [ ] There is a benchmark for any new code, or changes to existing code. - [ ] If this interacts with the agent in a new way, a system test has been added. - [ ] New code is free of linting errors. You can check this by running `make lint` locally. - [ ] New code doesn't break existing tests. You can check this by running `make test` locally. - [ ] Add an appropriate team label so this PR gets put in the right place for the release notes. - [x] All generated files are up to date. You can check this by running `make generate` locally. - [ ] Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild. Make sure all nested modules are up to date by running `make fix-modules` locally. Unsure? Have a question? Request a review! Co-authored-by: dario.castane <dario.castane@datadoghq.com>
darccio
changed the title
BenchmarksBenchmark execution time: 2026-07-10 16:30:14 Comparing candidate commit 1698dcb in PR branch Found 16 performance improvements and 1 performance regressions! Performance is the same for 307 metrics, 2 unstable metrics, 1 flaky benchmarks without significant changes.
|
rarguelloF
reviewed
Jul 13, 2026
| // Note: fasthttp.Server.ListenAndServe returns nil on graceful shutdown, | ||
| // Note: fasthttp.Server.Serve returns nil on graceful shutdown, | ||
| // not http.ErrServerClosed like net/http does. | ||
| go func() { _ = tc.Server.ListenAndServe(tc.Addr) }() |
Contributor
There was a problem hiding this comment.
It seems this change has uncovered an existing problem we had in the gofiber aspect, since in theory we are trying to prevent double-instrumentation from fasthttp with what we did there but we are seeing fasthttp spans in CI.
It seems the assertions were changed with this commit to actually expect the fasthttp span.
I'm not sure why we originally wanted fiber to not instrument fasthttp, since this is something we do for the rest of net/http based libraries, but its definitely not working for all scenarios so we should either remove that from the aspect and accept the double instrumentation, or actually fix it and make it work for this scenario (and other scenarios we might have missed).
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.
What does this PR do?
Backports #4999, #5005 and #5010. Includes #5018 to avoid flakiness.