Carry the intercepted HTTP status as an int, rendering it on demand - #12395
Carry the intercepted HTTP status as an int, rendering it on demand#12395dougqh wants to merge 1 commit into
Conversation
Metadata held the intercepted HTTP status as a UTF8BytesString, rendered unconditionally at construction. Every span with a status therefore paid a RadixTreeCache probe and carried a string, whether or not its serializer wanted one -- and a serializer that wants the number had nothing to ask for. That is the wrong default now that OTLP is one of the consumers: semantic conventions type http.response.status_code as an integer, so the string is precisely what it cannot use. Carry the int instead, and offer both accessors: getHttpStatusCode() returns the status (UNSET_STATUS when the span carries none, matching DDSpanContext's own convention), getHttpStatusCodeString() renders it through RadixTreeCache.HTTP_STATUSES for the string-typed protocols. Rendering moves from construction to the one call site that needs it, so the presence checks the mappers already perform become int comparisons and a numeric encoder skips the probe entirely. No wire change: every existing consumer asks for the string exactly where it did before. Emitting the status numerically -- and under its OpenTelemetry name -- is the follow-on this unblocks.
dougqh
added
comp: core
This comment has been minimized.
This comment has been minimized.
🟢 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. |
| measured, | ||
| topLevel, | ||
| httpStatusCode == 0 ? null : HTTP_STATUSES.get(httpStatusCode), | ||
| httpStatusCode, |
There was a problem hiding this comment.
This should be a small performance improvement for the application threads, since we're avoiding an expensive cache lookup.
However given that this is more of a functional change and the necessary benchmark doesn't yet exist, I'm going to leave the benchmarking to another PR.
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The code passes the integer HTTP status from DDSpanContext to Metadata. Each changed writer checks UNSET_STATUS before it gets the cached string value, so the wire value does not change.
🤖 Datadog Autotest · Commit 04bb58a · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
2 participants
What Does This Do
Metadatacarried the intercepted HTTP status as aUTF8BytesString, rendered unconditionally when theMetadatawas constructed. This carries theintinstead and offers both accessors:int getHttpStatusCode()— the status,UNSET_STATUS(0) when the span carries none, matching the conventionDDSpanContextalready uses for the field.UTF8BytesString getHttpStatusCodeString()— rendered throughRadixTreeCache.HTTP_STATUSESfor the string-typed protocols.No wire change. Every existing consumer —
TraceMapperV0_4/V0_5/V1,CiTestCycleMapperV1,FileBasedPayloadDispatcher, both OTLP encoders — asks for the string in exactly the place it did before, so the bytes on the wire are identical. The presence checks those mappers already perform becomeintcomparisons.Motivation
Every span with a status paid a cache probe and carried a string, whether or not its serializer wanted one — and a serializer that wants the number had nothing to ask for.
That is the wrong default now that OTLP is a consumer: semantic conventions type
http.response.status_codeas an integer, so the rendered string is precisely what it cannot use. Emitting the status under its OpenTelemetry name is blocked on this, because the right key with a string value is worse for a semconv consumer than the un-renamed Datadog name.Rendering now happens at the one call site that needs it, so a numeric encoder skips the probe entirely.
Additional Notes
Emitting the status numerically, and under its OpenTelemetry name, is the follow-on this unblocks — see #12354, which deliberately leaves the status un-renamed for exactly this reason.
TraceMapperV1keeps aStringlocal on purpose: itswriteAttribute(Writable, String, Object)debug-logs any non-Stringvalue.New
MetadataTestcovers the accessor pair: the int round-trips, an absent status renders asnullrather than"0", and two spans with the same status share oneUTF8BytesString— the property that makes routing through the cache worth doing at all.Contributor Checklist
./gradlew spotlessApply:dd-trace-core:testgreen apart fromTracerConnectionReliabilityTest, which fails identically on a cleanmasterworktree here (it wants a local agent), andPendingTraceBufferTest, which passes in isolationtype:,comp:,tag: ai generatedandtag: no release noteslabels assignedJira ticket
N/A
🤖 Generated with Claude Code