feat(tracer): Update the TagsList infrastructure to allow a secondary OtelName - #8971
Open
zacharycmontoya wants to merge 11 commits into
Open
feat(tracer): Update the TagsList infrastructure to allow a secondary OtelName#8971zacharycmontoya wants to merge 11 commits into
zacharycmontoya wants to merge 11 commits into
Conversation
zacharycmontoya
added
area:tracer
![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: 0fe888ba1c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| if (openTelemetrySemanticsEnabled) | ||
| { | ||
| processor.Process(new TagItem<string>(""otel.k"", Id, IdOTelBytes)); |
There was a problem hiding this comment.
Whenever either new OTel-alias generator test runs, the generator emits fields such as IdOtelBytes, but this expected output references IdOTelBytes, so the golden comparison always fails. Running the filtered TagsListGeneratorTests produced the same casing failure on net6.0 through net10.0; make this reference and the equivalent nullable-int reference consistent with the generated field name.
Useful? React with 👍 / 👎.
Comment on lines
+141
to
142
| var rawHttpStatusCode = span.GetHttpStatusCodeString(); | ||
| if (!string.IsNullOrEmpty(rawHttpStatusCode) && !TraceUtil.IsValidStatusCode(span.GetHttpStatusCode())) |
There was a problem hiding this comment.
When a span uses the default TagsList rather than an IHasStatusCode implementation and contains only an invalid http.response.status_code value, this new fallback detects that value, but the branch still clears only http.status_code. The invalid OTel attribute therefore remains on the span and is serialized despite normalization; clear the alias that matched, or clear both physical keys.
Useful? React with 👍 / 👎.
…emantics) and a OTelName property on the TagAttribute. This also updates the interface method ITags.EnumerateTags<TProcessor>() and its consumers. Now, when a TagsList provides each tag as key-value pair, it will provide the TagName as the key if openTelemetrySemanticsEnabled==false, otherwise it will provide the OTelName as the key. Notes: - None of these changes apply to MetricAttribute - This commit does _not_ update the generated tags implementations (that will be a follow-up commit - This commit does not apply the OTelName to any tags objects
…and add unit tests to demonstrate that setting/unsetting tags with the strong-typing or either tag name works and the last edit overrides an existing backing value. Affected tags classes are: - AwsSdkTags - HttpTags - InferredProxyTags - WebTags
- Add Tags.HttpResponseStatusCode to IsKnownWebTag to avoid carrying over a 'http.response.status_code' set on Activity in .NET 8+ - Implement the updated ITags interface in test class Datadog.Trace.Security.Unit.Tests.IAST.TestTags
- Update OtlpTracesJsonSerializer and OtlpTracesProtobufSerializer so they do not have their own stateful openTelemetrySemanticsEnabled setting. Instead, always rely on the setting provided by the Span - In product code and test code, unify on Span|MockSpan.GetHttpStatusCodeString() to return a string with either the tag contents of Tags.HttpStatusCode or Tags.HttpResponseStatusCode
zacharycmontoya
force-pushed
the
otel-tag-name
branch
from
July 31, 2026 19:10
0fe888b to
807e7ff
Compare
- StatsTests: group.HttpStatusCode is an int, so keep comparing against GetHttpStatusCode() rather than the string-returning GetHttpStatusCodeString() - TagsListGeneratorTests: fix OTelBytes/OtelBytes casing typo in expected generator output so it matches the generator's actual naming
Two Append calls used an escaped \n instead of an embedded literal newline. Every other newline in this generator relies on git's line-ending checkout normalization matching between Sources.cs and the test's expected strings, so a hardcoded LF only diverges from the checked-out CRLF on Windows, breaking CanGenerateTagsListWithOTelAlias and CanGenerateTagsListWithOTelAliasForNullableIntTag there.
BenchmarksBenchmark execution time: 2026-07-31 21:21:05 Comparing candidate commit 9712b4a in PR branch Found 0 performance improvements and 2 performance regressions! Performance is the same for 70 metrics, 0 unstable metrics, 67 known flaky benchmarks, 59 flaky benchmarks without significant changes.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8971) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8971) - mean (73ms) : 69, 78
master - mean (72ms) : 69, 75
section Bailout
This PR (8971) - mean (76ms) : 74, 77
master - mean (81ms) : 73, 89
section CallTarget+Inlining+NGEN
This PR (8971) - mean (1,097ms) : 1034, 1160
master - mean (1,096ms) : 1052, 1141
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8971) - mean (112ms) : 106, 118
master - mean (115ms) : 108, 121
section Bailout
This PR (8971) - mean (111ms) : 109, 113
master - mean (115ms) : 108, 122
section CallTarget+Inlining+NGEN
This PR (8971) - mean (791ms) : 765, 818
master - mean (785ms) : 763, 807
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8971) - mean (99ms) : 95, 104
master - mean (103ms) : 97, 109
section Bailout
This PR (8971) - mean (104ms) : 99, 108
master - mean (99ms) : 96, 103
section CallTarget+Inlining+NGEN
This PR (8971) - mean (945ms) : 901, 989
master - mean (950ms) : 901, 999
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8971) - mean (96ms) : 93, 99
master - mean (98ms) : 92, 105
section Bailout
This PR (8971) - mean (97ms) : 95, 99
master - mean (102ms) : 96, 108
section CallTarget+Inlining+NGEN
This PR (8971) - mean (827ms) : 780, 874
master - mean (822ms) : 782, 862
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8971) - mean (208ms) : 203, 212
master - mean (211ms) : 205, 217
section Bailout
This PR (8971) - mean (211ms) : 208, 214
master - mean (215ms) : 211, 219
section CallTarget+Inlining+NGEN
This PR (8971) - mean (1,241ms) : 1198, 1284
master - mean (1,268ms) : 1214, 1321
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8971) - mean (297ms) : 290, 304
master - mean (303ms) : 294, 312
section Bailout
This PR (8971) - mean (297ms) : 291, 303
master - mean (305ms) : 295, 315
section CallTarget+Inlining+NGEN
This PR (8971) - mean (996ms) : 972, 1019
master - mean (1,009ms) : 984, 1035
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8971) - mean (290ms) : 284, 296
master - mean (298ms) : 291, 305
section Bailout
This PR (8971) - mean (291ms) : 284, 298
master - mean (297ms) : 291, 303
section CallTarget+Inlining+NGEN
This PR (8971) - mean (1,189ms) : 1142, 1235
master - mean (1,200ms) : 1160, 1239
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8971) - mean (289ms) : 281, 296
master - mean (300ms) : 294, 306
section Bailout
This PR (8971) - mean (289ms) : 283, 295
master - mean (301ms) : 293, 308
section CallTarget+Inlining+NGEN
This PR (8971) - mean (1,064ms) : 1016, 1112
master - mean (1,094ms) : 989, 1199
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
andrewlock
approved these changes
Aug 3, 2026
andrewlock
left a comment
andrewlock
left a comment
Member
There was a problem hiding this comment.
LGTM, just a couple of minor questions/clarifications
| @@ -32,6 +32,11 @@ public TagAttribute(string tagName) => | |||
| /// Gets the name of the datadog tag the property corresponds to | |||
Member
There was a problem hiding this comment.
In the case where a tag is only used by Otel, we'd still set TagName. I'm wondering if it will confuse anyone 🤔 Maybe we could/should call it out explicitly in doc comments for TagName? WDYT?
| private static ReadOnlySpan<byte> HttpStatusCodeBytes => [176, 104, 116, 116, 112, 46, 115, 116, 97, 116, 117, 115, 95, 99, 111, 100, 101]; | ||
|
|
||
| // HttpStatusCodeOTelBytes = MessagePack.Serialize("http.response.status_code"); | ||
| private static ReadOnlySpan<byte> HttpStatusCodeOTelBytes => [185, 104, 116, 116, 112, 46, 114, 101, 115, 112, 111, 110, 115, 101, 46, 115, 116, 97, 116, 117, 115, 95, 99, 111, 100, 101]; |
Member
There was a problem hiding this comment.
Thinking out loud: Seeing as these all map to the same seqeuence of bytes I assume the compiler only embeds them in the assembly once, and maps everything to the same sequence, but maybe we should check?
Comment on lines
75
to
+76
| "http.status_code" => HttpStatusCode is null ? null : Datadog.Trace.Util.IntStringCache.ToInvariantString(HttpStatusCode.Value), | ||
| "http.response.status_code" => HttpStatusCode is null ? null : Datadog.Trace.Util.IntStringCache.ToInvariantString(HttpStatusCode.Value), |
Member
There was a problem hiding this comment.
Should we keep this like this, or change it to be the more compact/less duplicatey version?
Suggested change
| "http.status_code" => HttpStatusCode is null ? null : Datadog.Trace.Util.IntStringCache.ToInvariantString(HttpStatusCode.Value), | |
| "http.response.status_code" => HttpStatusCode is null ? null : Datadog.Trace.Util.IntStringCache.ToInvariantString(HttpStatusCode.Value), | |
| "http.status_code" or "http.response.status_code" => HttpStatusCode is null ? null : Datadog.Trace.Util.IntStringCache.ToInvariantString(HttpStatusCode.Value), |
| } | ||
| } | ||
|
|
||
| internal static string GetHttpStatusCodeString(this Span span) |
Member
There was a problem hiding this comment.
Should this be called by production code, or is it just for testing? If the latter, add the attribute to guard access
Suggested change
| internal static string GetHttpStatusCodeString(this Span span) | |
| [TestingAndPrivateOnly] | |
| internal static string GetHttpStatusCodeString(this Span span) |
Member
There was a problem hiding this comment.
Looks like it's called by NormalizeTraceProcessor, so I guess it is meant to be called. Makes me wonder if we could/should try to refactor the trace processor, but I'm not sure, and meh 😅
| /// Note: When a tag has both a DD and OTel name and is strongly typed in our ITags implementations, | ||
| /// there are two diverging behaviors: | ||
| /// - When applying a filter tag, the search checks both DD and OTel tag keys (and the singular tag value) | ||
| /// - When applying a filter tag regex, the search only checks one tag key (DD or Otel) based on the span's configured semantics setting. |
Member
There was a problem hiding this comment.
We could tweak that, so that it applies to both cases, at the expense of an additional enumeration for every span, regardless of the setting. Given the performance impact for everyone, and the generally low usage, I think the current approach makes total sense, even though it is potentially a bit confusing.
| { | ||
| if (openTelemetrySemanticsEnabled) | ||
| { | ||
| processor.Process(new TagItem<string>(""otel.k"", Id, IdOTelBytes)); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Updates the ITags infrastructure so a single ITags property can support generating a tag with either the Datadog-specific tag name or a secondary OpenTelemetry-specific tag. This allows our integrations and cross-product features to read/write a concept without depending on the "physical" tag name in
GetTag/SetTag.Reason for change
We want to support emitting spans that align with OpenTelemetry semantic conventions. Since we already store some shared concepts with ITags properties, we can use the strongly-typed properties to store the underlying information and then emit the tag with the requested semantics (Datadog vs OpenTelemetry). This also means that we readers/writers do not have to update their code with hard-coded tag names (e.g. DD
http.status_codevs OTelhttp.response.status_code) when accessing contextual data via spans -- instead they can access the corresponding strongly-typed property.Implementation details
Note: This PR is best reviewed commit-by-commit due to the large number of generated files and correctness refactors.
TagsListGenerator
TagAttributeclass now has an optionalOtelNamestring propertyOtelNameproperty receives the sameGetTag/SetTag/cached key bytes just like theTagName` propertyEnumerateTags<TProcessor>now has abool openTelemetrySemanticsEnabledparameter. For generated ITags implementationsbool openTelemetrySemanticsEnabledparameter toEnumerateTags<TProcessor>that indicates whether the Datadog or OpenTeleme, which carries the key to be emitted when OpenTelemetry semantics are enabled. The TagsListGenerator updateTags updated
The following classes use the
OtelNameto handle emitting the OpenTelemetry attributehttp.response.status_code:AwsSdTagsHttpTagsInferredProxyTagsWebTagsAdditional changes
SpanExtensionsandMockSpanExtensionsto use the methodGetHttpStatusCodeString()for extract the HTTP status code tags with astringreturn valueAspNetCoreHttpRequestHandlerhas been updated to make sure thathttp.response.status_codeis not copied from the Activity into our backing span, which is set on .NET 8+Test coverage
tracer/test/Datadog.Trace.SourceGenerators.Tests/TagsListGeneratorTests.csadds unit tests to cover the TagsListGenerator changestracer/test/Datadog.Trace.Tests/Tagging/TagsListTests.csadds unit tests to demonstrate the changes by assertingWebTags.HttpStatusCodecan be read/written through the property and the two tag keyshttp.status_codeandhttp.response.status_codeOther details
N/A