Refactoring and improvements to HttpHeaderHelpers#8153
Conversation
904a182 to
fe76568
Compare
tracer/src/Datadog.Trace/DataStreamsMonitoring/Transport/DataStreamsHttpHeaderHelper.cs
Outdated
Show resolved
Hide resolved
tracer/src/Datadog.Trace/HttpOverStreams/TraceAgentHttpHeaderHelper.cs
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
…e in passing in a lambda to reduce allocations, so just pass an instance in directly instead
They're stateless objects, so it doesn't really make sense to have them being created multiple times. Also, it avoids re-doing the concatenation work in the lazy etc
…as it allocates and doesn't do anything
fe76568 to
ec79d12
Compare
tonyredondo
left a comment
There was a problem hiding this comment.
nice to se a lot of new being removed.
BenchmarksBenchmark execution time: 2026-02-04 14:09:05 Comparing candidate commit dc55da9 in PR branch Found 5 performance improvements and 11 performance regressions! Performance is the same for 158 metrics, 18 unstable metrics. scenario:Benchmarks.Trace.AgentWriterBenchmark.WriteAndFlushEnrichedTraces net6.0
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleMoreComplexBody net6.0
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecEncoderBenchmark.EncodeLegacyArgs net6.0
scenario:Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces netcoreapp3.1
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSlice netcoreapp3.1
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSliceWithPool net6.0
scenario:Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark netcoreapp3.1
scenario:Benchmarks.Trace.Log4netBenchmark.EnrichedLog netcoreapp3.1
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishScope net6.0
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishSpan net472
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishSpan netcoreapp3.1
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishTwoScopes net472
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishTwoScopes netcoreapp3.1
|
| new KeyValuePair<string, string>(TelemetryConstants.ClientLibraryVersionHeader, TracerConstants.AssemblyVersion), | ||
| new KeyValuePair<string, string>(HttpHeaderNames.TracingEnabled, "false"), // don't add automatic instrumentation to requests directed to the agent | ||
| }; | ||
| => |
There was a problem hiding this comment.
In this case, we are creating new values for every call, right? Maybe this would be better?
internal static KeyValuePair<string, string>[] DefaultAgentHeaders { get; } =
[
new(TelemetryConstants.ClientLibraryLanguageHeader, TracerConstants.Language),
new(TelemetryConstants.ClientLibraryVersionHeader, TracerConstants.AssemblyVersion),
new(HttpHeaderNames.TracingEnabled, "false"),
];
There was a problem hiding this comment.
BTW, not in this PR but DataStreamsHttpHeaderNames also creates new values for each GetDefaultAgentHeaders call
There was a problem hiding this comment.
Thanks, I actually had the same thought, and did that originally, but changed my mind 😅
I decided against the property + initializer because then it will always be created, even if we never invoke this method, causing uneccessary allocation. And in practice, we only call this method once, so it shouldn't be an issue. That's especially enforced by making the types that use them singletons 😄
Summary of changes
Various usability and performance improvements to
HttpHeaderHelperReason for change
HttpHeaderHelperis used in conjunction withAgentTransportStrategy, for the cases where we're using our customDatadogHttpClientinstead of the built-inHttpClientetc. However, it's a bit clunky to use, and the way we're doing things at the moment causes a bunch of additional allocations on startup which we can avoid.The PR is a linear set of self-contained commits, so feel free to either review one-by-one, or just go for it and review the whole lot.
Implementation details
Lazy<T>for this and accept it. We want to be lazy because we only ever call this property if we're using our customer clientStringBuilderto build the lazy values instead of linq and interpolationFunc<T>calls, to reduce some allocationsuri => urilambdas as pointless allocation that we can handle elsewhereHttpHeaderHelperand a collection of default headers, expose the default headers directly onHttpHeaderHelper, as these must always stay in sync anyway.ExposureApithat wasn't matching the two sets of headers it sent (see above point!)ContentTypeproperty fromHttpHeaderHelperBaseas it's never usedMultipartAgentHeaderHelper, as it's now identical toMinimalAgentHeaderHelperMetadataHeaderstoHttpSerializedDefaultHeaders(as more descriptive)Test coverage
Added some unit tests to ensure all the
HttpHeaderHelperimplementations return the same headers (and the expected values) whether using theDefaultHeadersproperty or the privateHttpSerializedDefaultHeadersproperty