Repository navigation
Color known tags into co-occurrence slots in their ids (phase 3a - groundwork - dense tag storage) - #12882
Color known tags into co-occurrence slots in their ids (phase 3a - groundwork - dense tag storage)#12882dougqh wants to merge 6 commits into
Conversation
🟢 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. |
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0faed2d8ef
ℹ️ 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".
There was a problem hiding this comment.
The new assertion rejects a valid 63-slot registry even though slots 0–62 leave bit 63 reserved. Current conventions use 24 slots, so this defect affects the supported upper boundary.
🤖 Bits Code Review · Commit 0faed2d
| // An occupancy bit of 1L << slotOf(id) needs no branch for NO_SLOT: a long shift uses only the | ||
| // low 6 bits of its count, so 0xFFFF lands on bit 63, which no real slot uses | ||
| assertEquals(1L << 63, 1L << KnownTagCodec.slotOf(KnownTags.RESOURCE_NAME_ID)); | ||
| assertTrue(KnownTagCodec.slotCount() < 63, "real slots must stay below the shared bit 63"); |
There was a problem hiding this comment.
Allow the supported 63-slot count in the assertion
When conventions require exactly 63 slots, the generator permits indexes 0–62 and emits SLOT_COUNT = 63, leaving bit 63 reserved. This assertion nevertheless fails, rejecting a valid registry at the supported limit. Allow a count of 63.
| assertTrue(KnownTagCodec.slotCount() < 63, "real slots must stay below the shared bit 63"); | |
| assertTrue(KnownTagCodec.slotCount() <= 63, "real slots must stay below the shared bit 63"); |
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
Code origin is the debugger's add-on, set on entry spans (and their local root) and @trace spans, not on every span. It moves from base to a code_origin mixin on http.server, rpc.server, messaging.consumer and internal, which also takes it out of every client, DB and AWS clique. Only _dd.code_origin.type is registered. The frame tags are _dd.code_origin.frames.<N>.<field>, a name pattern: an unbounded family can't have ids ahead of time, so it can't use the dense store, and the registry doesn't model patterns yet. Frame 0's six constants are listed as never registered, with that reason. (Only frame 0 is produced today.) With #12882's coloring the 164 tags use 40 slots, down from 47. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…atterns in the coverage test These integrations' tags were thought unused, but only their constants were: Elasticsearch, Twilio and JSP set them as string literals, and Quartz and Hystrix use the constants. Each becomes a Java span type (set sites checked per tag): - elasticsearch.rest and elasticsearch.transport (db.client): the transport client's request, response and shard tags; the REST client's body and params, and its http.method and http.url. - twilio.sdk (http.client), jsp.compile (internal), jsp.render (view.render), quartz.job and hystrix.command (internal). Six names have no constant and are registered anyway: elasticsearch.action, .request, .body, .params, .shard.replication.successful, and jsp.requestURL. Tags set only by literal are outside the coverage test's reach; APMLP-2046 proposes auditing them in the instrumentation tests. The coverage test now matches entries as patterns (`*`): - NEVER_REGISTERED: a pattern excludes all it matches (Tags.SPAN_KIND_*, Tags.LLMOBS_*_SPAN_KIND, the code-origin frames). - NOT_YET_REGISTERED_FAMILIES: a family, with a reason, excusing only its unregistered constants (Tags.TEST_*, Tags.GIT_*, Tags.CI_*, DDTags.CI_*, DDTags.TEST_*: CI Visibility, in a follow-up PR); stale once none of its constants is unregistered. - NOT_YET_REGISTERED: the remaining single constants, still a strict ratchet. 26 single constants remain, plus the CI Visibility families. With #12882's coloring the 203 tags use 55 slots (elasticsearch.transport). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
db.client carried system-specific tags in every DB span's clique. System-specific tags extend one concept, so they become types: - db.client keeps what any database client sets: type, instance, operation, user, statement, and a command's outcome (db.command.cancelled and .results.count, set by Redis and Memcached). - db.sql (JDBC, R2DBC): db.pool.name, db.schema, db.warehouse. - db.cassandra, db.couchbase (seed nodes, bucket, operation id), db.redis (db.redis.dbIndex), and db.mongo. - Database Monitoring is a product add-on, so its propagation tags become a dbm mixin on db.sql and db.mongo, where it injects comments. With #12882's coloring the 213 tags use 47 slots, down from 57 (elasticsearch.transport). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
More details
Slot encoding remains compatible with serial-based tag lookups. The interception and direction-dependent naming changes in the supplied comparison also yielded no reportable source-level finding.
🤖 Bits Code Review · Commit 1a63fa0
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>
1a63fa0 to
06bac77
Compare
#12882 was rebased onto master after #12817 merged, and gained two commits (a custom tag's zero id is unslotted; a slot bit is only a may-be-present signal). This merge's tree is the rebased #12882 plus exactly this branch's own changes (0faed2d..8610d72), which apply to it without conflicts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Two review findings, both confirmed in the code: - RabbitChannelInstrumentation calls onPeerConnection on its client, producer and consumer decorators. messaging.producer and messaging.client now include peer, peer_address and outbound_peer (peer.service is computed for client and producer spans, as for http.client and rpc.client); messaging.consumer includes peer_address and inbound_peer. Consumers also set peer.hostname, which outbound_peer declares for client spans only: a TODO, as for websocket spans. - Code origin's entry capture instruments annotated controller methods, whose active span is the framework's handler span, so the code_origin mixin also applies to web.handler. Slots unchanged at 47 with #12882's coloring. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A span type can carry more tags than the occupancy long has slots: a CI Visibility test span carries about 100. Most of those are set only some of the time (retries, ITR, pull request info, ...), and falling back to NO_SLOT would put them all on bit 63, which is then set on most spans and turns every check of those tags into a scan. `slot: shared` marks a tag as rarely set. The coloring keeps a shared tag apart from every tag that isn't shared in its cliques, but lets two shared tags take the same slot. Tags that aren't shared are colored first, so they take the low, exact slots; shared tags fill the slots left free. A span type then fits as long as its always-present tags do. With no tag marked shared, the coloring and every id are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Replaces the `slot: shared` marker with the requirement level the conventions already carry, so slots stay out of the registry's vocabulary. A ref can override the level per span type, so a tag can be always present on one span type and occasional on another. Tags that are required, conditional or recommended on some span type are colored first and always get exact slots. Optional and opt_in tags get exact slots too while any are free; only when a span type runs out do two of them share one, on the slot the fewest already hold. Coloring fails only if a span type's non-optional tags alone need more than 63. On the current conventions nothing overflows: SLOT_COUNT stays 24 and no tag shares a slot, though some slots move with the new order. KnownTags marks a tag that shares a slot with `+ shares slot`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An optional tag pushed onto a shared slot by one span type's overflow could land on the slot of an optional tag of another span type, one that fits. Optional tags now get soft edges only on span types with more tags than slots, so a span type that fits never has two tags on one slot. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Replaces the `slot: shared` markers with requirement levels. The tags TestDecoratorImpl.afterStart puts on every CI span (the CI provider and commit) and the test source range are recommended rather than optional, so the coloring keeps them on exact slots; the tags set only some of the time stay optional and absorb the overflow on test spans. With #12882's coloring, the 306 tags fit in 63 slots; only the CI test span types, which carry 73 to 104 tags, have tags sharing a slot. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What Does This Do
No functional change. Tag id values change, because each id now carries a slot, but nothing in the tracer reads slots yet, and hashing, iteration order and tag identity are unaffected.
Gives every known tag a slot in bits [47-32] of its id, assigned at build time by graph coloring. Tags that appear together on a span type get distinct slots, so
1L << slotOf(id)gives a span's tags their own presence bits in onelongwhen instrumentation follows its span type.This is split out of the dense store (#12856) for ease of review, and to enable similar existence-check optimizations in the span builder's tag ledger.
Slots don't limit what a span can hold. The dense store still allows two tags with the same slot on one span. A slot is only a fast path for the common case: when instrumentation follows its span type, the tags it sets are colored apart. So a clear bit means a tag is definitely absent, while a set bit means it's probably present and the store confirms with a scan.
TagMap).colorSlotscolors the co-occurrence graph greedily: tags that arerequired,conditionalorrecommendedon some span type go first, then the rest; within each group, most-constrained first, then by name, so ids stay a pure function of the conventions. Tags that never share a span type can share a slot, which keeps the count bounded by the largest clique.NO_SLOT(0xFFFF).longwith bit 63 left over.1L << slotOf(id)needs no branch forNO_SLOT: a long shift uses only the low 6 bits of its count, so 0xFFFF lands on bit 63, which every unslotted tag shares.optionaloropt_inthere may share a slot once the free slots run out, on the slot the fewest of them already hold. Its other tags keep exact slots, and a span type that fits never has two tags on one slot. The build fails only if a span type's non-optional tags alone need more than 63. GeneratedKnownTagsmarks a tag that shares a slot with+ shares slot.KnownTagCodec:SLOT_SHIFT,NO_SLOT,slotOf,hasSlot(false for a custom tag's0Lid), andslotCount()(the generatedSLOT_COUNT).tag-assignment.txtgains a slot column, and each generated id's comment shows its slot.How slots are assigned: an example
Here's the current output for three span types. Slots 0–7 hold the non-optional tags every one of them has, and slots 18–21 the optional ones, which are colored last; each of those tags gets the same slot everywhere. In between, the span types diverge, and each slot is reused by tags that usually don't appear on the same span:
http.serverhttp.clientdb.client_dd.integration,_dd.parent_id,component,error.message,error.stack,error.type,service,span.kindhttp.client_ip_dd.peer.service.remapped_from_dd.peer.service.remapped_fromhttp.hostname_dd.peer.service.source_dd.peer.service.sourcehttp.methodhttp.methoddb.instancehttp.status_codehttp.status_codedb.operationhttp.urlhttp.urldb.statementnetwork.protocol.versionnetwork.protocol.versiondb.typehttp.query.stringpeer.hostnamepeer.hostnamehttp.routepeer.servicepeer.servicehttp.useragenthttp.resend_countdb.usernetwork.client.ipdb.pool.name_dd.svc_src,peer.ipv4,peer.ipv6,peer.portservlet.contextservlet.pathhttp.methodis 10 on both HTTP spans, andpeer.hostnameis 14 on both client spans.http.query.stringon a server span butpeer.hostnameon a client span.envis 9,versionis 12. They live in a separateTagMap, so they never share a word with span tags.http.serveralone has 24 tags, which all need distinct slots, so the greedy coloring is optimal for the current conventions. Nothing overflows, so no tag shares a slot.Motivation
Per-span structures want a compact presence word for known tags:
TagMap.Ledger(span builder tags) could answersmartRemoveof a never-set tag with one bit test instead of canonicalizing the name and scanning, and could keep one change per slot.A presence bit keyed by
serial & 63would collide arbitrarily, and most often on exactly the spans with the most tags. Coloring keeps the bits distinct for tags that usually occur together, so collisions only come from tags set outside their span type.Additional Notes
makeTagId(serial)still builds a slot-0, flag-free id: it's for serial-keyed lookups such asnameOf, and its doc now says not to compare it with a real id. I checked every caller; none does.tag-assignment.txtshows each tag's slot. The modeled span types today arehttp.server,http.client,db.clientandview.render.🤖 Generated with Claude Code