Repository navigation
Conversation
TagMap now holds its tags in two internal stores: an IdStore for known tags by id and a name store for custom tags by name. In hash mode, the only mode so far, one HashStore serves as both, so the split adds no object per tag. The bucket structure, read-through to frozen parents and removal tombstones move into HashStore unchanged; TagMap delegates to it. The public API and behavior are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TagRouting resolves a tag name once, by KnownTagCodec.keyOrAliasOf, and routes the operation to the store that holds it: a known tag to the id store by its id, anything else to the name store by its name. TagMap's name-keyed getters, setters, removals and entry factories, and its resolved variants, all go through it, so name-to-id resolution lives in one place. A direction-scoped alias is still stored as a custom tag. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DDSpanContext now owns a HashStore (and, outside hash mode, an IdStore) instead of a TagMap, created when the span is constructed and read through to the frozen tracer-level tags. Every tag write and read goes through TagRouting under one lock, the HashStore itself. Direction, alias, interception and pending-alias handling stay in DDSpanContext and DirectionalTags, now applied to the stores. HashStore is no longer an IdStore: it is the name-keyed hash, which also holds known tags by id, and exposes an IdStore adapter over them. The static HASH_MODE flag lets hash mode call the hash directly, so a span needs no adapter object. Internal readers (tag post-processors, the serializer) get a short-lived TagMap view over the span's stores whose name-keyed reads honor the span's direction; getTags() is still a snapshot copy. TagMap.Internals is gone: the store API is the direct path. Its tests move to TagStoresTest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TagRouting.putAll built a TagMap view to reuse TagMap.putAll, but putAll is too big to inline, so the view escaped and cost an allocation on the span's tag-seeding path. In hash mode it now copies into the HashStore directly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The view handed to the post-processor chain escapes, so building one per use cost an allocation at trace write and another at serialization. The span now creates one lazily, under its tag lock, and reuses it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dougqh
added
comp: core
A direction-scoped alias written before the span has a kind is no longer stored in the span's HashStore. It goes into a small holder that DirectionalTags owns, which stays null until the first such write, so the HashStore holds only custom tags. Kind-set resolution walks only the holder, and once every alias has resolved the holder is dropped, so the span returns to the fast path rather than staying flagged for life. The id setters' routing gate now tests the holder for null instead of reading a sticky flag. Reads of a tag with an alias probe the holder only when there is one. Last-write-wins and the removal rules are unchanged. Emitting the span's tags (trace-write and serialization processing, and the getTags snapshot) copies the holder's entries in under their literal names, so a span that never learns its kind still emits them. Co-Authored-By: Claude Opus 5.5 <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. |
The tag-registry generator now assigns each known tag a slot in id bits [47-32] by graph coloring: each concrete span type's resolved tags, and the trace-level tags, form a clique and get distinct slots. So 1L << slotOf(id) is an exact presence bit for a span's tags, and one long holds them all. Tags on no modeled span type get NO_SLOT, which a long shift maps onto bit 63; real slots are capped below it. KnownTagCodec gains SLOT_SHIFT, NO_SLOT, slotOf, hasSlot and slotCount. Nothing in the tracer reads slots yet: this is split out of the dense store (#12856) so the store, the span builder's tag ledger and a map-level routing summary can each build on it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
hasSlot(0L) decoded slot 0, so the id keyOf gives a custom tag looked like the known tag in slot 0. It now returns false for 0L. The slot-count test also allows the full 63 slots the generator permits. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A span can still hold two tags that share a slot, so slotOf's presence bit is exact only when it is clear. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each half of a TagMap now has its own bucket array. HashIdStore holds the known tags by id, with a count, a final parent, and slot masks from the tag ids' co-occurrence slots: a clear occupancy bit is a definite miss, and the parent's slots let a read-through miss stop early. Removing an inherited known tag puts that tag's shared, immutable tombstone entry in the local buckets, so it allocates nothing. HashStore holds only custom tags and is created on the first custom write; until then a child borrows its parent's custom store read-only. The raw bucket mechanics move into a stateless HashSupport whose operations report what they did through return values. HASH_MODE is gone. A span now keeps its tags in a TagMap it creates with the span (TagMap.forSpan), locks on it, and routes to it through TagRouting; that TagMap is what its internal readers get, replacing the cached view. DirectionalTags' store-only methods are removed, and its test now exercises the methods the span uses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DDSpanContext no longer routes its reads and writes through a TagMap. It owns its id store, created with the span, and its custom store, created on the first custom write and until then null or borrowed from the read-through parent. Every span read and write goes span to store, and the span locks on its id store, which always exists. The TagMap is again only a cached view for the span's tag post-processors, the serializer and the getTags() snapshot. Before each use the span points it at its current custom store; afterwards it adopts any custom store a reader created through the view. DirectionalTags works on the span's stores through a small TagStores interface the span implements; its test supplies one over a TagMap. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
process_id, _dd.trace_span_attribute_schema and _dd.agent_psr are now declared at the trace level, beside language and runtime-id, with the types the tracer writes them with (long, int and double). Every local root span carries them, so as custom tags they made each root span create a custom tag store; as known tags they go to its id store. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DDSpanContext no longer implements a store interface for DirectionalTags or exposes store operations publicly. DirectionalTags now takes the span's id store and its custom store, which may be null or borrowed from the read-through parent. It never creates the custom store: a removal that would have to hide a tag in a borrowed store leaves it untouched and returns true, and the span creates its own store and repeats the removal, which is idempotent. An alias write that resolves to its tag now leaves removing the alias from the custom store to the span. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The id setters now supersede a pending alias themselves, before routing, instead of routing every write of an aliased tag through TagInterceptor while the span held one. TagInterceptor routes on INTERCEPTED alone again and no longer knows aliases exist; DDSpanContext.supersedeAliases is private. The check comes first as hasScopedAlias(id), so for a constant id without an alias it folds away entirely. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TagMap.DirectionSource is gone. The span's cached view now gets the span's direction and whether it may hold pending aliases as two plain fields, set by attach() alongside its current custom store before each use, and a TagMap of its own reads every name as written (direction unknown). DDSpanContext no longer implements an interface for its view, so tagDirection() and hasPendingAliases() are no longer public; its public surface matches master. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
HashIdStore's lookups (reads, the find before a removal, and the read-through visibility check) now match an entry on its 64-bit id through HashSupport.findById, so they no longer resolve the tag's name with nameOf or compare names. Every id-store entry is keyed by id, and the stored bucket hash still filters before the id compare. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The private helpers that send a tag to the known or the custom store are now routeSet, routePut, routePutAll and routeRemoval (from setResolved / storeTag, putEntry, putAllTags and removeStored), so a write that picks its store reads differently from one that already has it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
|
||
| @Override | ||
| public Object getValue(long tagId) { | ||
| Entry entry = this.getEntryReader(tagId); |
There was a problem hiding this comment.
To Claude - let's also add getEntry just so this reads a bit more clearly
DenseIdStore was the helper's only user, so its by-name and Map.Entry modes were dead. It becomes DenseIdStore.DenseReader, an id-only flyweight: tagId() is a field read and entry() needs no branch, with only the name resolved lazily. TAG_ID_NOT_COMPUTED goes with it. IdStore.Cursor becomes an interface, so DenseCursor extends DenseReader and returns itself from next(): one object per iteration instead of a cursor plus a helper. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| private Entry nextEntry; | ||
| // Two phases: the id store's visible entries, through its own cursor, then the custom store's | ||
| // chain -- the leaf's buckets first, then each ancestor's in turn (read-through union). | ||
| private IdStore.Cursor ids; // null once the id phase is done |
There was a problem hiding this comment.
To Claude - let's call this idCursor
From review: TagMap's ids / names / namesBorrowed are now knownStore / nameStore / nameStoreBorrowed (and namesForWrite is nameStoreForWrite); the iterator's id cursor is knownCursor. toInternalString's local StringBuilders are builder rather than ledger. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The interception lambdas in seedAbsentTags and setAllTags passed `this` where the base passed `ctx`, which made each a capturing lambda allocated per call; ctx is this, so they pass ctx again. HashIdStore's and HashStore's visibleParentCount counted through forEachParent with an int[] counter, allocated on every size() of a store with a parent (per serialized span); they count with a plain loop again, as the base did. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- isPresent reads the reader's own tagId instead of resolving the name again. - createFromParent delegates to readThroughParent, which now carries the exact-emptiness comment; TagMap(TagMap) uses IdStore.create and HashStore.inheritedFrom; nameStoreForWrite uses HashStore.create. - HashSupport.checkBuckets indexes a bucket group's entries by j, not the bucket index i, so the integrity check really inspects groups (copied as-is from the old TagMap.checkIntegrity). - DDSpanContext's constructors drop the tagsSize parameter, which nothing used once the span owned its stores, and CoreTracer no longer computes it per span (that included a size() walking the read-through parent). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Only its own test called it; the span reads values from its stores directly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- isPresent moves above setTag(String, Object)'s Javadoc, which it had been inserted under, so setTag is documented again. - A leftover constructor doc stacked on HashStore.inheritedFrom's is removed. - TagMap.EMPTY's comment described the old length-1 bucket array; it now points at IdStoreFactory, which explains why building EMPTY is safe during class initialization. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Span-level and trace-level tags are colored independently, so their slot numbers overlap, and a local root span holds both in one store. With one occupancy word, a trace-level tag (runtime-id) made the slot of an unrelated span-level tag (http.method) look occupied, so a miss on the root span fell through to a scan: GetTagBenchmark's missing known tag read was 6.1 ns dense against 3.2 ns hash. IdStore keeps spanOccupancy and traceOccupancy and picks the word by the tag's level (a select that folds for a constant id). Tombstones keep one shared mask with their own level set. Costs 8 bytes per store. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 0f1e5acd2a
ℹ️ 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".
| while (level != null) { | ||
| for (++this.index; this.index < level.used; ++this.index) { | ||
| if (this.leaf.readIfVisible(level, this.index, this)) { | ||
| return this; |
There was a problem hiding this comment.
When DD_TRACE_EXPERIMENTAL_DENSE_TAGS_ENABLED is enabled, TagMap.iterator() exposes this cursor directly, and every advance returns the same mutable DenseCursor. Consumers that retain iterator results—such as collecting map.stream() into a list—therefore receive multiple aliases that all report the last visited tag and value, unlike the stable entries returned in hash mode. Materialize a stable reader or Entry at the public iterator boundary.
Useful? React with 👍 / 👎.
| "negative, where KnownTagCodec's lookup sentinels live" | ||
| } | ||
| require(slot == NO_SLOT || slot in 0 until MAX_SLOTS) { "slot $slot is outside [0, $MAX_SLOTS)" } | ||
| var id = (serial.toLong() shl 48) or (slot.toLong() shl SLOT_SHIFT) |
There was a problem hiding this comment.
Generated IDs now include the slot bits here, but KnownTagCodec.makeTagId(serial) still returns only serial << 48. For a slotted tag, that documented ID constructor produces a value accepted by isKnown and nameOf but unequal to the generated constant; in hash mode, setting a TagMap with that value stores it under the partial ID and subsequent lookup by name or the real ID misses it, while dense mode happens to work because it stores only the serial. Update the runtime constructor or ensure partial IDs are rejected rather than treated as usable known-tag IDs.
Useful? React with 👍 / 👎.
| if (value == null) { | ||
| return null; |
There was a problem hiding this comment.
When dense tags are enabled, writing a known key with a null value through the Map API—for example TagMap.put(name, null) or TagMap.fromMap(...)—stores a slot and increments the map size, but this method then treats that stored value as if the key were absent. The resulting map can report size() == 1 while containsKey(name) is false, and removal returns null despite deleting the slot; hash mode previously retained the null mapping. Use a distinct null-value sentinel or reject null before mutating the store.
Useful? React with 👍 / 👎.
| /** Stores {@code entry}, sharing its Entry where the store keeps Entry objects. */ | ||
| private void routePut(TagMap.EntryReader entryReader) { | ||
| if (entryReader.tagId() != 0) { | ||
| knownTags.put(entryReader); // no Entry built for a dense store, or for a flyweight from one |
There was a problem hiding this comment.
When DDSpanContext.setTag or setMetric receives a non-null EntryReader whose entry() is null, this direct store route now throws in HashIdStore.put or HashStore.put. Before the split these methods delegated to TagMap.set(EntryReader), whose documented behavior treats both a null reader and a reader with no entry as a no-op. Resolve and null-check the entry before routing so span sinks preserve that contract.
Useful? React with 👍 / 👎.
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two critical correctness gaps exist in the dense-storage TagMap implementation: concurrent reads on a frozen map share a mutable reader, allowing tag values to bleed across threads; and stored null values are indistinguishable from absent entries, breaking containsKey, getOrDefault, and removal semantics relative to hash mode. A third issue affects sequential traversal: the iterator exposes a single reused cursor, so buffering or collecting the stream loses all but the final entry.
🤖 Bits Code Review · Commit 0f1e5ac
|
|
||
| @Override | ||
| public EntryReader getEntryReader(long tagId) { | ||
| return this.readerOf(tagId, this.getValue(tagId)); |
There was a problem hiding this comment.
With dense storage enabled, concurrent reads from the same frozen TagMap share a mutable reader. Another thread can overwrite it before TagMap.getEntry() copies its contents, so concurrent getString("component") and getString("http.route") calls can return each other's values despite no map writes. Return a stable, per-call result for point reads.
| return this.readerOf(tagId, this.getValue(tagId)); | |
| Object value = this.getValue(tagId); | |
| return value == null ? null : TagMap.Entry.newAnyEntry(tagId, value); |
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
| DenseIdStore level = this.level; | ||
| while (level != null) { | ||
| for (++this.index; this.index < level.used; ++this.index) { | ||
| if (this.leaf.readIfVisible(level, this.index, this)) { |
There was a problem hiding this comment.
With dense storage enabled, TagMap.iterator() exposes the same mutable cursor for every known tag. Collecting a two-tag stream into a list therefore retains two references to the final tag, and buffering operations such as sorted() lose earlier entries. Materialize stable entries at public traversal boundaries, including the iterator and related consumer paths, while keeping flyweights internal.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
|
|
||
| /** The flyweight over {@code value}, or null when there is none. */ | ||
| private EntryReader readerOf(long tagId, Object value) { | ||
| if (value == null) { |
There was a problem hiding this comment.
With dense storage enabled, TagMap.put("http.route", null) stores and counts the mapping, but readerOf() treats its null value as absence. The map consequently reports size 1 and exposes the entry while containsKey("http.route") returns false, unlike the base and hash implementations. Preserve presence separately from the value throughout lookup, replacement, and removal handling.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
This branch has not been deployed
1 participant
What Does This Do
Splits tag storage into strict id and name stores, and has the span own them directly.
TagMapbecomes a thin view for everyone else. No behavior change.The idea: classify a tag name once, at the boundary (known tag → id, custom tag → name, direction-scoped alias → the span's call), then keep the two kinds apart inside.
Layers
KnownTagCodecHashStoreIdStoreHashIdStore) now; a dense store slots in hereTagRoutingDDSpanContextHashStore(also its one lock) and, outside hash mode, anIdStore. All setting logic lives here, withDirectionalTagsandTagInterceptorTagMapNotable changes
TagMap.Internalsis gone. The strict store API is the direct path, so core no longer needs a trusted side door aroundTagMap. Its tests move toTagStoresTest.HASH_MODEis a compile-time constant (currentlytrue). Each id operation isHASH_MODE ? hash.x(id) : ids.x(id), so hash mode has no adapter object and no extra load.TagMapview over the span's stores.getTags()is still a snapshot copy.Entryobjects and clone onlyBucketGroupchains, as before.Motivation
TagMaphad accreted name canonicalization, known-versus-custom classification and, with #12731, direction awareness. That forced core to reach around it throughInternals, and every write route repeated the known-versus-custom and direction logic. Most of #12731's review findings were one write route missing a rule another route had.This puts policy back in the span layer, keeps storage simple and fast, and gives a dense store a clean place to replace the known-tag half. That matters for the biggest remaining allocation sites: point reads building an
Entryper read, primitive metrics stored as boxedEntrys, and the dense store's id arrays.Additional Notes
Performance (JDK 17,
-f 2,-prof gc; base is #12731's tip6aa6d4f1e2):SetTag.knownTag_byIdSetTag.knownTag_byNameSetTag.customTag_byNameGetTag.knownTag_unsafeGetTagGetTag.knownTag_getTagSpanCreation.webServerSpanSpanCreation.bareStartSpan(4 forks)Throughput is within noise. With the adapter instead of the folded
HASH_MODEpath,unsafeGetTagwas +6% and bare id writes +4%.Known cost: +40 B per span. The span's view, handed to the tag post-processor chain, escapes there and isn't scalar-replaced, so the view is cached per span (32 B plus a field). This is a stopgap: the post-processors are due for rework, and once they read the span's stores directly, the view and the field go away. Without the cache it was +32 B on the app thread and another +32 B at serialization.
Semantic note: name-keyed reads through a span's view resolve aliases by the span's direction, where today's raw
unsafeTagsreads don't. No current post-processor reads an alias name.Draft: stacked on #12731's branch, so this diff shows only the split. A follow-up commit moves pending aliases into a small lazily created holder: the custom store then holds only custom tags, and the pending-alias check becomes a null check that resets after resolution.
🤖 Generated with Claude Code