Update OtlpSnapshotHelper in place; migrate remaining traces-only OTLP suites off Docker - #9118
Merged
chojomok merged 4 commits intoAug 25, 2026
Conversation
…f Docker
Alternate approach to the additive one in otlp-http-mocktracer: instead
of keeping the JToken-based OtlpSnapshotHelper methods alongside new
typed ones, update them in place (MergeDatadogRequests and
GetAttributeStringValue now take the typed MockOtlp model; the old
JToken overloads and the unused SetAttributeStringValue are deleted).
OpenTelemetryWebRequestTests and OpenTelemetryHttpClientTests are fully
migrated off the Docker ddapm-test-agent onto MockTracerAgent -- both
verified passing locally against unmodified snapshots.
OpenTelemetrySdkTests.SubmitsOtlpTraces is migrated too (its sibling
methods SubmitsOtlpMetrics/SubmitsOtlpRuntimeMetrics/SubmitsOtlpLogs
stay on Docker: gRPC + metrics/logs decoding are still non-goals for
MockTracerAgent's OTLP support), but its existing snapshots do NOT yet
match:
- doubleValue formatting differs ("1" vs "1.0") -- Google.Protobuf's
JsonFormatter renders whole-number doubles without a trailing ".0",
the old ddapm-agent rendering (or the scrubber pipeline) apparently
preserved it.
- Span content/order mismatch in the non-Datadog-SDK (otelTracesEnabled)
branch -- span identity looks scrambled relative to the verified
file, needs investigation (likely a merge/sort or scope-grouping
difference between the old JToken-array-of-requests shape and the
new typed-then-formatted approach for that branch specifically).
Still TODO before this is mergeable:
- Root-cause and fix both diffs above.
- Decide whether to regenerate OpenTelemetrySdkTests.SubmitsOtlpTraces*
snapshots (likely required for the doubleValue formatting change,
which is a legitimate representation difference, not a bug) vs. fix
in code (the span-order issue is likely a real bug to fix, not a
snapshot update).
- OpenTelemetrySdkTests.SubmitsOtlpTraces only spot-tested with
packageVersion="" (default); not yet run across the full
GetOtlpTracesTestData() matrix (multiple OTel SDK package versions).
- Not yet pushed / no PR opened for this branch.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two real bugs found while validating the migration, both fixed: - MergeDatadogRequests's default span sort used StringComparer.Ordinal, but the original JToken-based implementation used a bare OrderBy (culture-aware default comparer). Ordinal reorders names that differ only by the case of a leading letter (e.g. "SomeSpan" vs "some.name"), which looked like a real span-identity mismatch against existing snapshots. Dropped the explicit comparer to match the original default exactly. - AddProtobufToJsonScrubbers (which converts protobuf's enum-as-string rendering, e.g. "SPAN_KIND_INTERNAL", to the int form existing snapshots expect) was only called for http/protobuf rows. Since MockTracerAgent always re-serializes via Google.Protobuf's JsonFormatter regardless of which wire protocol the request arrived over, that mismatch now shows up on every row, not just protobuf ones -- call it unconditionally, matching every other migrated suite. Also added a scrubber for a real formatting difference: JsonFormatter renders a whole-number double as "1" rather than the "1.0" existing snapshots expect (both are valid representations of the same double value). All 8 SubmitsOtlpTraces rows (all protocol/backup/semantics combinations) now pass against the existing, unmodified snapshots. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
chojomok
added
AI Generated
bouwkast
removed
the
AI Generated
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
chojomok
commented
Aug 25, 2026
| var settings = VerifyHelper.GetSpanVerifierSettings(); | ||
| settings.AddRegexScrubber(_exceptionStacktraceOtlpRegex, @"string_value"": ""System.ArgumentException: Example argument exception"""); | ||
| settings.AddRegexScrubber(_exceptionStacktraceOtlpJsonRegex, @"stringValue"": ""System.ArgumentException: Example argument exception"""); | ||
| settings.AddRegexScrubber(_wholeNumberDoubleValueRegex, @"""doubleValue"": $1.0"); |
Collaborator
Author
There was a problem hiding this comment.
This scrubber isn't masking real data loss — it's reconciling two valid textual representations of the same double value.
Google.Protobuf's JsonFormatter renders a whole-number double as 1 instead of 1.0 (matching how e.g. (1.0).ToString() behaves in C#). The existing .verified.txt snapshots were captured against a renderer that always kept the trailing .0. Both 1 and 1.0 parse back to the identical IEEE-754 double, so nothing is being truncated.
Evidence this is purely cosmetic: doubles with a genuine fractional part (e.g. 1.5) already matched the snapshots with zero scrubbing needed — the regex only ever fires on whole numbers missing the decimal point, never on values with real precision to lose.
BenchmarksBenchmark execution time: 2026-08-25 18:38:18 Comparing candidate commit 0fde821 in PR branch Found 1 performance improvements and 1 performance regressions! Performance is the same for 70 metrics, 0 unstable metrics, 66 known flaky benchmarks, 60 flaky benchmarks without significant changes.
|
Keep wording close to main where it already existed; tighten new comments to be proportional to the code they explain. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
Stacked on #9097 (base branch
otlp-http-mocktracer). This is the "optional follow-up" that PR called out, taking a different implementation approach than the additive one used there:OtlpSnapshotHelper's methods in place instead of adding parallel typed methods alongside the JToken-based ones:MergeDatadogRequestsandGetAttributeStringValuenow take the typedMockOtlpmodel directly; the old JToken overloads and the unusedSetAttributeStringValueare deleted. Normalization (NormalizeResourceAttributes/NormalizeSpans/etc.) still operates on the final JSON, since it writes placeholder text into fields (trace/span IDs) whose real protobuf types can't hold arbitrary strings — that part is unchanged and still shared with any remaining JSON/Docker consumer.OpenTelemetryWebRequestTestsandOpenTelemetryHttpClientTestsfully off the Dockerddapm-test-agentontoMockTracerAgent's OTLP support (neither drives a Kestrel fixture — both launch their own sample process directly and now point OTLP export at the mock agent's own port before launch).OpenTelemetrySdkTests.SubmitsOtlpTracesthe same way. Its three sibling methods (SubmitsOtlpMetrics,SubmitsOtlpRuntimeMetrics,SubmitsOtlpLogs) stay on Docker — they exercise gRPC and metrics/logs decoding, both explicit non-goals ofMockTracerAgent's OTLP support, and none of them callOtlpSnapshotHelperat all, so they're unaffected by the signature changes.MockTracerAgent.WaitForOtlpTraceRequestsAsync(in the baseotlp-http-mocktracerbranch already) as the entry point all four migrated suites use.Fixes found while validating
Two real bugs surfaced while getting existing snapshots to match unmodified:
MergeDatadogRequests's default span sort accidentally usedStringComparer.Ordinal; the original implementation used a bareOrderBy(culture-aware default), and switching comparers reordered names differing only by leading-letter case.AddProtobufToJsonScrubbers(enum-as-string → int) was only called forhttp/protobufrows; since everything now round-trips through the sameJsonFormatterrendering regardless of original wire protocol, it needs to run unconditionally.Also added a scrubber for whole-number
doubleValueformatting ("1"vs"1.0"— both valid representations of the same double).Test plan
OpenTelemetryWebRequestTests.SubmitsOtlpTraces(2 cases) — pass against existing, unmodified snapshots.OpenTelemetryHttpClientTests.SubmitsOtlpTraces(2 cases) — pass against existing, unmodified snapshots.OpenTelemetrySdkTests.SubmitsOtlpTraces(8 cases, all protocol/backup/semantics combinations,packageVersion="") — pass against existing, unmodified snapshots.OtlpAspNetCoreMvc31Tests/OtlpAspNetCoreMinimalApisTests(108 cases) re-verified alongside the above — no regression.OpenTelemetrySdkTests.SubmitsOtlpTracesnot yet run across the fullGetOtlpTracesTestData()matrix (multiple OTel SDK package versions) — only spot-tested with the default version locally; should be verified in CI.🤖 Generated with Claude Code