[dotnet] Enable flag eval metrics tests#6689
[dotnet] Enable flag eval metrics tests#6689gh-worker-dd-mergequeue-cf854d[bot] merged 9 commits into
Conversation
|
|
🎉 All green!❄️ No new flaky tests detected 🔗 Commit SHA: ad85a71 | Docs | Datadog PR Page | Give us feedback! |
| { | ||
| value = request.VariationType?.ToUpper() switch | ||
| { | ||
| "BOOLEAN" => await _client.GetBooleanValueAsync(request.Flag, Convert.ToBoolean(request.DefaultValue), context), |
There was a problem hiding this comment.
Problem
The EvaluateRequest class has a DefaultValue property typed as object:
public class EvaluateRequest
{
public object DefaultValue { get; set; } // Can be bool, int, string, etc.
}When ASP.NET Core's System.Text.Json deserializes JSON into an object property, it does not convert to native .NET types. Instead, it wraps the value in a JsonElement:
{ "defaultValue": true } // Becomes JsonElement, not bool
{ "defaultValue": 42 } // Becomes JsonElement, not int
{ "defaultValue": "hello" } // Becomes JsonElement, not stringThe original code assumed native types:
Convert.ToBoolean(request.DefaultValue) // Throws! DefaultValue is JsonElementConvert.ToBoolean() has no knowledge of JsonElement and throws InvalidCastException.
Why Previous Tests Didn't Fail
The original code had a catch block that swallowed all exceptions:
try
{
value = ... Convert.ToBoolean(request.DefaultValue) ...
}
catch (Exception ex)
{
value = request.DefaultValue;
reason = "ERROR";
}Previous tests (test_dynamic_evaluation.py, test_exposures.py) were asserting on:
- The evaluated flag value (returned by the endpoint)
- The reason field in the response
When InvalidCastException was thrown, the catch block returned reason = "ERROR" and value = request.DefaultValue. If a test expected an error case or didn't care about the reason, it would pass.
Why Flag Eval Metrics Tests Failed
The metrics tests need OpenFeature hooks to fire. The exception was thrown before calling the OpenFeature API:
Convert.ToBoolean(JsonElement)throws- The
catchblock catches it and returns"ERROR" - OpenFeature is never called → hooks never fire → no metrics emitted
The tests assert on metrics received by the OTLP intake, not on the HTTP response. No OpenFeature call = no hooks = no metrics = test failure.
The Fix
Check if the value is a JsonElement and extract the actual value:
private static bool GetDefaultValueAsBool(object defaultValue)
{
if (defaultValue is JsonElement jsonElement)
{
return jsonElement.ValueKind switch
{
JsonValueKind.True => true,
JsonValueKind.False => false,
JsonValueKind.String => bool.Parse(jsonElement.GetString()),
_ => false
};
}
return Convert.ToBoolean(defaultValue); // Fallback for non-JsonElement
}136dee4 to
84f94a9
Compare
84f94a9 to
05a4aed
Compare
|
Leaving the test disabled for now because the changes in DataDog/dd-trace-dotnet#8367 haven't been published yet. Tests pass when run this locally: DataDog/dd-trace-dotnet#8367 (comment) Copy-pasted from the referenced comment:
|
| RUN apt-get update && DEBIAN_FRONTEND=noninteractive apt-get install -y curl | ||
|
|
||
| # install dd-trace-dotnet (must be done before setting LD_PRELOAD) | ||
| COPY utils/build/docker/dotnet/install_ddtrace.sh binaries/ /binaries/ |
There was a problem hiding this comment.
Why run COPY binaries/ commands twice?
Stage 1: build-app (SDK image)
FROM mcr.microsoft.com/dotnet/sdk:8.0 AS build-app
# NEW: Copy binaries for NuGet package resolution
COPY binaries/ /binaries/
RUN if ls /binaries/*.nupkg 1> /dev/null 2>&1; then \
dotnet nuget add source /binaries --name local-packages; \
fi
# NuGet packages are resolved HERE
RUN dotnet restore
RUN dotnet publishPurpose: Build the weblog application. NuGet packages (like Datadog.FeatureFlags.OpenFeature.*.nupkg) must be available as a NuGet source before dotnet restore runs.
Stage 2: runtime (ASP.NET image)
FROM mcr.microsoft.com/dotnet/aspnet:8.0 AS runtime
# EXISTING: Copy binaries for tracer installation
COPY utils/build/docker/dotnet/install_ddtrace.sh binaries/ /binaries/
RUN /binaries/install_ddtrace.shPurpose: Install the Datadog tracer into the runtime image. The install_ddtrace.sh script uses:
- Native profiler:
Datadog.Trace.ClrProfiler.Native.so - Managed tracer DLLs:
net6.0/Datadog.Trace.dll, etc.
nccatoni
left a comment
There was a problem hiding this comment.
LGTM (for @DataDog/system-tests-core) but you should get a review from someone more familiar with the weblog
System.Text.Json deserializes JSON values into Dictionary<string, object> as JsonElement wrappers rather than native strings. The previous code used `attr.Value as string` which returned null for JsonElement values, causing targeting rules to fail. This fix properly extracts string values from JsonElement, enabling targeting rule evaluation to work correctly.
Remove irrelevant skips for: - Test_FFE_Eval_Metric_Parse_Error_Invalid_Regex - Test_FFE_Eval_No_Config_Loaded The .NET SDK uses a managed evaluator (not libdatadog), and now correctly returns PARSE_ERROR for invalid regex patterns and PROVIDER_NOT_READY when no config is loaded. Related: DataDog/dd-trace-dotnet#8367
- Add local NuGet source from binaries folder if nupkg files present - Update OpenFeature to 2.3.0 (required by Datadog.FeatureFlags.OpenFeature 2.0.1) - Use wildcard version for Datadog.FeatureFlags.OpenFeature to pick up local builds
The flag eval metrics feature is implemented in dd-trace-dotnet PR #8367 but not yet released. Mark as missing_feature until v3.42.0 is published.
bf0159e to
ef91458
Compare
a2caf44 to
5654890
Compare
a9d24b8 to
5e836ba
Compare
5e836ba to
855e812
Compare
|
The tests pass when I run locally with the artifacts in https://dev.azure.com/datadoghq/dd-trace-dotnet/_build/results?buildId=200327&view=artifacts&pathAsName=false&type=publishedArtifacts I believe the failures in CI will resolve after DataDog/dd-trace-dotnet#8367 is merged and a new package is published. If I understand correctly, CI Docker image only includes the tracer tar.gz and the weblog restores Datadog.FeatureFlags.OpenFeature from nuget.org, which doesn't have the metrics hook yet |
cbeauchesne
left a comment
There was a problem hiding this comment.
You'll nee to remove the branch flag in PR title and re-run the CI to be able to merge it.
## Summary of changes
Adds flag evaluation metrics support to the Datadog OpenFeature
provider. When a feature flag is evaluated, a counter metric is emitted
with relevant attributes.
## Reason for change
Part of the Feature Flags Evaluation (FFE) initiative to provide
visibility into flag evaluations across all Datadog server SDKs. This
brings parity with implementations in dd-trace-py and dd-trace-go.
## Implementation details
- **FlagEvalMetrics.cs**: Core metrics recording class using
`System.Diagnostics.Metrics`
- Meter name: `Datadog.FeatureFlags.OpenFeature`
- Metric: `feature_flag.evaluations` (Counter<long>)
- Unit: `{evaluation}`
- Tags: `feature_flag.key`, `feature_flag.result.variant`,
`feature_flag.result.reason`, `error.type` (optional),
`feature_flag.result.allocation_key` (optional)
- **FlagEvalMetricsHook.cs**: OpenFeature hook that implements
`FinallyAsync` to record metrics after each evaluation completes
- Converts reason to lowercase for consistency
- Maps `ErrorType` enum to snake_case strings
- Extracts allocation key from flag metadata when present
- **DatadogProvider.cs**: Registers the metrics hook with the
OpenFeature API
- Updated OpenFeature SDK dependency from 2.0.0 to 2.3.0
- **Package version bumped to 2.3.0** to match OpenFeature SDK
dependency version. This signals the breaking changes from OpenFeature
2.3.0:
- .NET 6 support dropped (now requires .NET 8+)
- Hook `finally` signature changed to include evaluation details
**Conditional compilation**: Only enabled for .NET 6+ (`#if
NET6_0_OR_GREATER`) since `System.Diagnostics.Metrics` requires .NET 6+.
## Test coverage
- Feature is validated through system-tests
(`tests/ffe/test_flag_eval_metrics.py`)
- Also checked in DataDog/ffe-dogfooding#56
- Unit tests were explored but deferred due to type conflicts between
shared source files in `Datadog.FeatureFlags.OpenFeature` and
`Datadog.Trace` assemblies
## Other details
Jira: https://datadoghq.atlassian.net/browse/FFL-1946
Related PRs:
- dd-trace-py: DataDog/dd-trace-py#17029
- dd-trace-go: DataDog/dd-trace-go#3381
- system-tests: DataDog/system-tests#6689
---------
Co-authored-by: Andrew Lock <andrew.lock@datadoghq.com>
…shed The tests pass locally but CI fails because Datadog.FeatureFlags.OpenFeature 2.3.0 hasn't been published to NuGet.org yet (current version is 2.0.1). Tracked by FFL-2257.
| tests/ffe/test_dynamic_evaluation.py: v3.36.0 | ||
| tests/ffe/test_exposures.py: v3.36.0 | ||
| tests/ffe/test_flag_eval_metrics.py: missing_feature | ||
| tests/ffe/test_flag_eval_metrics.py: missing_feature (FFL-2257 - Requires Datadog.FeatureFlags.OpenFeature 2.3.0 NuGet package to be published) |
There was a problem hiding this comment.
The -dev suffix doesn't work if the feature depends are NuGet package dependency updates, which are pulled from NuGet.org. The latest_snapshot Docker image doesn't include the .nupkg files.
I imagine the proper fix is to update create-system-test-docker-base-images.yml to include .nupkg files in the image, but I wasn't planning to make anything more than minor changes to the existing testing infra, so I think the simplest thing for now is to disable the test and wait for the official release to enable it
There was a problem hiding this comment.
I've verified that the tests pass locally for now
Motivation
Prepare the .NET weblog for
test_flag_eval_metrics.pytests to validate the newfeature_flag.evaluationsOTel metric implementation.Related PRs:
Changes
Weblog (
utils/build/docker/dotnet/weblog/):Fix
JsonElementhandling inFeatureFlagEvaluatorController.csSystem.Text.Jsondeserializesobjectproperties asJsonElement, not native typesJsonElementbefore calling OpenFeature APIsInvalidCastExceptionthat was occurring before OpenFeature hooks could fireUpgrade OpenFeature packages in
app.csproj:OpenFeature: 2.0.0 → 2.3.0 (required forFinallyAsynchook signature withFlagEvaluationDetails<T>)Datadog.FeatureFlags.OpenFeature: Use wildcard version to pick up local/CI buildsNote: The
tests/ffe/test_flag_eval_metrics.pytests remain marked asmissing_featurein the manifest until dd-trace-dotnet v3.42.0 is released with flag eval metrics support.Workflow
🚀 Once your PR is reviewed and the CI green, you can merge it!
🛟 #apm-shared-testing 🛟
Reviewer checklist
tests/ormanifests/is modified ? I have the approval from R&P teambuild-XXX-imagelabel is present