Repository navigation
Make a null tag value remove the tag in TagMap (dense store - part 1) - #12999
Conversation
setTag(tag, null) already removes a tag from a span, but TagMap's own setters stored a null-valued entry. Internal writers that go straight to a TagMap could therefore leave null tags on a span, and the serializer sent them as a msgpack nil under meta. TagMap.put / set / getAndSet (by name or id, and putAll / fromMap through set) now remove the tag on a null value and return its prior value. A null Entry or reader is still a no-op. The one writer that relied on null tags was payload tagging: a JSON null in a request or response body produced a tag with a null value. It now produces no tag, so such a value also no longer counts toward the max-tags limit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dougqh
added
type: bug fix
🟢 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. |
dougqh
changed the title
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: fa9b441840
ℹ️ 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".
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
A repeated JSON property ending in null retains its earlier value as a payload tag. Restoring the null setter call clears that stale value through TagMap’s new removal semantics.
🤖 Bits Code Review · Commit fa9b441
With null now meaning remove, a JSON null set no tag at all, so a
duplicate key such as {"x":"old","x":null} left the stale x=old tag.
Setting null again removes it, as overwriting with null did before.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sarahchen6
left a comment
sarahchen6
left a comment
There was a problem hiding this comment.
one typo nit but LGTM
Co-authored-by: Sarah Chen <sarah.chen@datadoghq.com>
|
/merge |
|
View all feedbacks in Devflow UI.
It will be processed automatically as soon as GitHub reports it as mergeable. View in MergeQueue UI.
The expected merge time in
Build pipeline has failing jobs for 59ff42d: What to do next?
DetailsSince those jobs are not marked as being allowed to fail, the pipeline will most likely fail. |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
What Does This Do
A null value given to a
TagMapsetter now removes the tag, matching whatsetTag(tag, null)already does on a span.TagMap.put,setandgetAndSet, by name or by id, remove the tag and return its prior value.putAllandfromMapgo throughset, so they follow.EntryorEntryReaderis still a no-op, as before.nullin a request or response body now produces no tag. Before, it produced a tag with a null value.Motivation
TagMapand the span API disagreed about null. Writers that bypasssetTagand write to aTagMapdirectly could leave null-valued tags on a span, and the v0.4 serializer then sent them as a msgpacknilundermeta. Payload tagging was the one writer that did this.This is split out of the
TagMapstore work (#12918) so that the behavior change is reviewed on its own and the optimization PRs stacked on it change no behavior.Behavior change
Payload tagging only:
null(for examplep.j3.3from[1, 2, 3.14, null, true]) no longer emits a tag.respectMaxTagsLimit, that'sp.j3.4.Additional Notes
TagMapNullToleranceTestcases cover set by name, set by id,put,getAndSetandfromMap.TracerConnectionReliabilityTestcases, which need Docker.Where this is going
This PR is one step in replacing the span's single
TagMapwith storage that knows each tag's kind. The goal is to cut what every span allocates on the application thread, without changing behavior.The end state
TagMapbecomes a thin view for code outside the span.Entryobject per tag, and each span's store is presized from a per-operation hint. With the flag on, a web-request-shaped trace allocates about 30% less on the application thread than on master.TagStore, not aTagMap. That covers post-processors now and serializers later, so noTagMapview gets built for them.TagStorehas its own verbs,loadandstore, apart fromTagMap'sget/setand the span's interceptedgetTag/setTag, soDDSpanContextcan offer both. The tag stores themselves use the same verbs (Add id and name tag stores, not yet used (dense store - part 5) #13006). It starts with what the eager processors use, by name and by id, and grows typed-load and primitive-store variants. Its intercepted counterpart,SpanData(Take the span's tag lock once in setTagsFrom (tag api - part 1) #13030), is whatsetTagsFrom,TagContributorandTagExtractortake, with one lock per batch.Later, not in this stack: cheaper read-through to parent spans, trace-level tags in a per-trace holder (
TraceSegment), and primitive tag values without boxing.The stack. This replaces #12918, split into pieces that can each be reviewed, and measured, on their own.
TagMap. This is the stack's only behavior change, kept separate on purpose.TagMap, and addTraceShapeBenchmarkTagStorefor the eager post-processorsTagMap's bucket mechanics intoHashSupport(a pure move)TagMapa facade over the two storestrace.experimental.dense.tags.enabled(off by default)Every PR after #12999 is behavior-preserving.
The pieces' allocation figures come from a deterministic per-thread probe (
ThreadMXBeanallocated bytes) that builds the same 7-span trace asTraceShapeBenchmark. The #12992 and #12926 figures come fromTraceShapeBenchmarkitself.🤖 Generated with Claude Code