Merged
Conversation
BenchmarksBenchmark execution time: 2026-01-12 15:14:59 Comparing candidate commit 688c62c in PR branch Found 9 performance improvements and 9 performance regressions! Performance is the same for 158 metrics, 10 unstable metrics. scenario:Benchmarks.Trace.AgentWriterBenchmark.WriteAndFlushEnrichedTraces netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleSimpleBody netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody net6.0
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecWafBenchmark.RunWafRealisticBenchmarkWithAttack netcoreapp3.1
scenario:Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces net472
scenario:Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces net6.0
scenario:Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces netcoreapp3.1
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSliceWithPool net6.0
scenario:Benchmarks.Trace.DbCommandBenchmark.ExecuteNonQuery netcoreapp3.1
scenario:Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearch net6.0
scenario:Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearchAsync net6.0
scenario:Benchmarks.Trace.RedisBenchmark.SendReceive net472
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishScope net6.0
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishTwoScopes netcoreapp3.1
scenario:Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin net472
|
cba84d3 to
bda4ee9
Compare
This comment has been minimized.
This comment has been minimized.
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (7896) 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 (7896) - mean (69ms) : 67, 70
master - mean (69ms) : 67, 70
section Bailout
This PR (7896) - mean (72ms) : 71, 73
master - mean (73ms) : 72, 74
section CallTarget+Inlining+NGEN
This PR (7896) - mean (1,011ms) : 961, 1061
master - mean (1,017ms) : 961, 1073
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 (7896) - mean (106ms) : 103, 109
master - mean (106ms) : 104, 108
section Bailout
This PR (7896) - mean (107ms) : 106, 108
master - mean (107ms) : 106, 108
section CallTarget+Inlining+NGEN
This PR (7896) - mean (742ms) : 682, 802
master - mean (742ms) : 686, 798
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7896) - mean (94ms) : 91, 96
master - mean (94ms) : 92, 96
section Bailout
This PR (7896) - mean (94ms) : 93, 95
master - mean (94ms) : 93, 95
section CallTarget+Inlining+NGEN
This PR (7896) - mean (707ms) : 665, 749
master - mean (715ms) : 690, 740
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7896) - mean (92ms) : 89, 95
master - mean (92ms) : 90, 94
section Bailout
This PR (7896) - mean (93ms) : 92, 94
master - mean (93ms) : 92, 94
section CallTarget+Inlining+NGEN
This PR (7896) - mean (633ms) : 618, 648
master - mean (634ms) : 620, 647
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 (7896) - mean (193ms) : 186, 200
master - mean (192ms) : 188, 196
section Bailout
This PR (7896) - mean (195ms) : 193, 197
master - mean (195ms) : 193, 197
section CallTarget+Inlining+NGEN
This PR (7896) - mean (1,122ms) : 1066, 1179
master - mean (1,114ms) : 1062, 1165
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 (7896) - mean (276ms) : 271, 280
master - mean (275ms) : 270, 279
section Bailout
This PR (7896) - mean (277ms) : 272, 281
master - mean (275ms) : 271, 279
section CallTarget+Inlining+NGEN
This PR (7896) - mean (922ms) : 861, 984
master - mean (923ms) : 869, 976
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7896) - mean (269ms) : 264, 274
master - mean (269ms) : 264, 274
section Bailout
This PR (7896) - mean (268ms) : 265, 271
master - mean (267ms) : 263, 271
section CallTarget+Inlining+NGEN
This PR (7896) - mean (920ms) : 874, 966
master - mean (926ms) : 885, 966
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7896) - mean (268ms) : 263, 272
master - mean (268ms) : 261, 274
section Bailout
This PR (7896) - mean (267ms) : 263, 271
master - mean (267ms) : 264, 270
section CallTarget+Inlining+NGEN
This PR (7896) - mean (822ms) : 804, 841
master - mean (819ms) : 801, 836
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
dd-oleksii
reviewed
Dec 16, 2025
Member
dd-oleksii
left a comment
There was a problem hiding this comment.
Did a review pass, mostly focusing on evaluator. This is probably the cleanest evaluation outline that I've seen 👍
Main things to handle before release:
- We should error out on invalid configuration with
PARSE_ERRORinstead of trying to continue and serving potentially unexpected values. - Various conversion inconsistencies (see inline comments).
- Need to check that user-expected type matches
variationType. I'm not sure that usingTypeis a good approach here and all other SDKs use explicit enums. (Example whereTypecan fall short:stringcould be either a string or a JSON flag, and JSON flags could basically be anything.) - It looks like a lot of types and methods are public, which makes it hard to evolve SDK without breaking backward compatibility. We generally prefer exposing little to no surface except OpenFeature provider interface and configuration options.
I have started collecting some of the edge case requirements over here, so we're aligned between SDKs. If you have any questions or notice any requirements missing, just drop me a note and I'll add it 🙏
tracer/src/Datadog.Trace.Manual/FeatureFlags/FeatureFlagsSdk.cs
Outdated
Show resolved
Hide resolved
e-n-0
reviewed
Dec 18, 2025
e-n-0
approved these changes
Dec 18, 2025
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
andrewlock
requested changes
Dec 18, 2025
Member
andrewlock
left a comment
There was a problem hiding this comment.
Mostly just nits, but there's a few important aspects:
- As discussed, we should probably look at a different approach to supporting OpenFeature, instead of a NuGet package. To get this PR merged ASAP I would suggest
- Remove the new NuGet package, and just copy the DatadogProvider into the test project for now
- In a subsequent PR, explore the source generator approach
- We should add public API telemetry to the public APIs
- Do we need all of the public APIs that we've added? If the provider is the expect approach, can/should we just provide that. Because we can never remove these public APIs once they're added, but we can always add new ones later. I would prefer we kept them internal until we're sure they're the correct shape
- There are some problems around initialization and setting handling. We need to handle changes in setting values, and ensure we aren't calling
TracerSettings.FromDefaultSourcesor anything
tracer/src/Datadog.Trace/Configuration/supported-configurations-docs.yaml
Show resolved
Hide resolved
tracer/src/Datadog.Trace/FeatureFlags/Exposure/Model/Allocation.cs
Outdated
Show resolved
Hide resolved
tracer/src/Datadog.Trace/FeatureFlags/Exposure/Model/ExposureEvent.cs
Outdated
Show resolved
Hide resolved
tracer/test/test-applications/feature-flags/Samples.FeatureFlags/Program.cs
Outdated
Show resolved
Hide resolved
tracer/test/test-applications/integrations/Samples.FeatureFlags/Program.cs
Show resolved
Hide resolved
tracer/test/test-applications/feature-flags/Samples.OpenFeature-2.9/Program.cs
Outdated
Show resolved
Hide resolved
tracer/test/test-applications/feature-flags/Samples.OpenFeature-2.9/Program.cs
Outdated
Show resolved
Hide resolved
.../test/test-applications/feature-flags/Samples.OpenFeature-2.9/Samples.OpenFeature-2.9.csproj
Outdated
Show resolved
Hide resolved
tracer/src/Datadog.Trace.Manual/FeatureFlags/FeatureFlagsSdk.cs
Outdated
Show resolved
Hide resolved
70878df to
4bb7279
Compare
andrewlock
reviewed
Dec 23, 2025
Member
andrewlock
left a comment
There was a problem hiding this comment.
This is looking good! A bunch of nits really, but main feedback:
- We should avoid using
!in any new code, there's almost always ways around it, which we should use - Tweaked the config code
- We should think about the public API a little in terms of what we instrument for future proofing. I've added one suggestion
- Create a throw helper for
FormatException - Use concrete types where possible
And then for later:
- Public API telemetry
- Nuget/Source gen discussion for DatadogProvider
I haven't approved yet, just so I can keep track a bit better, but there's nothing specifically blocking now I think, just improvements we can make 😄
tracer/src/Datadog.Trace.Manual/FeatureFlags/FeatureFlagsSdk.cs
Outdated
Show resolved
Hide resolved
...ualInstrumentation/FeatureFlags/FeatureFlagsSdkRegisterOnNewConfigEventHandlerIntegration.cs
Outdated
Show resolved
Hide resolved
tracer/src/Datadog.Trace/Configuration/supported-configurations-docs.yaml
Show resolved
Hide resolved
tracer/src/Datadog.Trace.Manual/FeatureFlags/FeatureFlagsSdk.cs
Outdated
Show resolved
Hide resolved
231eaa5 to
9f485c7
Compare
It is actually easier to evolve this over time, but by instrumenting the exploded args we can maintain compatibility without extra duck typing
04cb92c to
688c62c
Compare
andrewlock
added a commit
that referenced
this pull request
Jan 15, 2026
## Summary of changes Use the new `PostAsJsonAsync` API to avoid serializing to string and byte array locally ## Reason for change We recently added support for `PostAsJsonAsync` in #8017, and this is a good example where we can easily swap it in in the new Exposures API used by FFE: #7896 ## Implementation details Just updated to use the new APIs. A couple of points to notice: - This doesn't enable gzip. If the endpoint supports it, we should probably enable that, but would need testing - This changes the serialization settings, by using `SerializationHelpers.DefaultJsonSettings`, which omits serializing null members, and uses snake-case serialization, instead of the PascalCase we have currently - However, [looking at the spec](https://github.com/DataDog/dd-source/blob/main/domains/evp-workers/apps/exposures-worker/schemas/batched_exposures.json), I'm pretty sure we're not compliant today, and what we're changing to is correct - Which suggests we're missing tests... ## Test coverage This _should_ be covered by existing tests... and if it's not, then we need to add those tests 😄
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
6 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
Add support for Feature Flag configuration frameworks
Reason for change
Spec document can be found here
Implementation details
Retrieved FF config from backend through Remote Config. Enable SDK functions.
Created
Tracer.FeatureFlags.OpenFeature.nupkgto add support for OpenFeature from V2.0.0 onwardsTest coverage
Added unit and integration tests
Other details