Pre-construct TagMap.Entry objects in InternalTagsAdder#11555
Conversation
InternalTagsAdder set base.service / version via TagMap.set(tag, value), allocating a fresh TagMap.Entry per span. Both values are fixed for the life of the tracer, and TagMap.Entry objects are safe to share across maps (the OptimizedTagMap collision design relies on it), so build the two Entry objects once in the constructor and reuse them via set(entry). A JFR profile of petclinic (2026-06-03) attributed ~52 allocation samples to InternalTagsAdder.processTags (one Entry per span); this drops them to zero. Re-applies the change from the stale PR #10965 (415 commits behind master, drifted signature) onto current master. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dd-octo-sts
Bot
added
the
tag: ai generated
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
|
![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: 934fd5dc3b
ℹ️ 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".
| this.version = version != null && !version.isEmpty() ? UTF8BytesString.create(version) : null; | ||
| this.baseServiceEntry = | ||
| ddService != null | ||
| ? TagMap.Entry.create(DDTags.BASE_SERVICE, UTF8BytesString.create(ddService)) |
There was a problem hiding this comment.
When DD_SERVICE is explicitly set to an empty string, this now becomes null because TagMap.Entry.create(String, CharSequence) returns null for empty values, while Config can still pass through an empty configured service name from the config provider. In that scenario a span whose service name is also empty used to reach the version branch and add DD_VERSION, but the new baseServiceEntry == null guard exits before versionEntry is considered, silently dropping the version tag for that configuration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in f5f5cc9. The constructor now keeps a separate ddService UTF8BytesString handle that drives both the early-return guard and the case-insensitive service-name comparison (independent of whether baseServiceEntry was prebuilt). For the empty-DD_SERVICE case, Entry.create still returns null, so the base.service branch falls back to unsafeTags.set(BASE_SERVICE, ddService) — preserving byte-identical behavior — and crucially the version branch is still reached when the span's service name also matches the empty configured service.
I added regression coverage in the migrated InternalTagsAdderTest (now JUnit 5): new InternalTagsAdder("", "1.0") with an empty span service still adds version 1.0, and with a non-empty span service it takes the base.service branch and adds no version.
(Reply by Claude)
dougqh
added
comp: core
Addresses the Codex review comment on #11555: pre-building the base.service Entry must not change behavior for an explicitly-empty DD_SERVICE. Entry.create rejects empty values, so baseServiceEntry is null in that case; the processTags branch now falls back to set(BASE_SERVICE, ddService) to preserve byte-identical behavior, and the version branch is still reached when the span service also matches the empty configured service. Migrate InternalTagsAdderTest from Groovy/Spock to JUnit 5 (parameterized with @MethodSource) and add regression coverage for the empty-DD_SERVICE case (9 migrated cases + 2 new = 11). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🟢 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. |
1 participant
What This Does
InternalTagsAddersets_dd.base_service/versionon spans viaTagMap.set(tag, value), which allocates a freshTagMap.Entryper span. Both values are fixed for the life of the tracer, andTagMap.Entryobjects are explicitly safe to share across maps (theOptimizedTagMapcollision design viaBucketGrouprelies on it). This pre-builds the twoEntryobjects once in the constructor and reuses them viaset(entry).Motivation
A JFR allocation profile of spring-petclinic under load (full agent, 2026-06-03) attributed ~52 allocation samples to
InternalTagsAdder.processTags— oneEntryallocated per span forbase.service. Tag-map handling (TagMap$Entry+OptimizedTagMap) was the single largest non-core tracer allocation theme in that profile, ahead of the CSS metrics system. This change drops theInternalTagsAddercontribution to zero.Notes
dd/pre-construct-tagmap-entries), which had drifted 415 commits behind master and changedprocessTags's signature; this version sits on current master with the unchangedAppendableSpanLinkssignature. Supersedes Reuse TagMap.Entry objects in InternalTagsAdder #10965.Entry.stringValue()for the OBJECT entry resolves toobj.toString()(cached), identical to the priorddService.toString()in theequalsIgnoreCasecomparison. The only edge case — emptyddServicenow yields an early return — is unreachable in production (Config.getServiceName()is never"").InternalTagsAdderTest.groovypasses unchanged.🤖 Generated with Claude Code