feat: Add trace when evaluating feature flags#755
Conversation
Signed-off-by: Kyle Julian <38759683+kylejuliandev@users.noreply.github.com>
Signed-off-by: Kyle Julian <38759683+kylejuliandev@users.noreply.github.com>
Signed-off-by: Kyle Julian <38759683+kylejuliandev@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #755 +/- ##
==========================================
+ Coverage 93.52% 93.61% +0.08%
==========================================
Files 69 70 +1
Lines 2981 3037 +56
Branches 355 374 +19
==========================================
+ Hits 2788 2843 +55
Misses 135 135
- Partials 58 59 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request implements OpenTelemetry tracing for feature flag evaluations in the OpenFeature client, including the addition of an activity source and corresponding unit tests. The review feedback highlights several discrepancies with OpenTelemetry Semantic Conventions, suggesting the use of standard attribute names (e.g., 'feature_flag.provider_name', 'feature_flag.evaluation_reason', 'feature_flag.evaluation_value', 'feature_flag.variant') and recommending that span statuses be explicitly set to 'Error' when failures occur to improve telemetry accuracy.
Signed-off-by: Kyle Julian <38759683+kylejuliandev@users.noreply.github.com>
Signed-off-by: Kyle Julian <38759683+kylejuliandev@users.noreply.github.com>
Signed-off-by: Kyle Julian <38759683+kylejuliandev@users.noreply.github.com>
Signed-off-by: Kyle Julian <38759683+kylejuliandev@users.noreply.github.com>
askpt
left a comment
There was a problem hiding this comment.
LGTM! I expect this change to be mentioned in a README since the consumer might need to add extra configuration to remove or add these traces.
cc: @beeme1mr, @open-feature/technical-steering-committee, @WeihanLi, @arttonoyan
| var provider = providerInfo.Item2; | ||
|
|
||
| var activity = OpenFeatureActivitySource.StartActivity("feature_flag.evaluation"); | ||
| activity?.SetTag("feature_flag.key", flagKey); |
There was a problem hiding this comment.
for the SetTag/SetStatus, think we could add tags in one place and add tags only when IsAllDataRequested
There was a problem hiding this comment.
Fixed in 6e943fa - this was a great spot! It helps avoid allocations in high performance scenarios
I wonder if we should add feature_flag.provider.name in all instances or only when IsAllDataRequested is true
|
maybe we want to deprecate the |
| var providerMetadata = provider.GetMetadata(); | ||
| if (providerMetadata != null) | ||
| { | ||
| activity?.SetTag("feature_flag.provider.name", providerMetadata.Name); |
There was a problem hiding this comment.
Would we like to keep these tagName/activityName to be constants in a separated place?
There was a problem hiding this comment.
Fixed in 6e943fa - moved the tag names (apart from error.type) to the OpenFeatureActivitySource class
| .ConfigureResource(resource => resource.AddService("openfeature-aspnetcore-sample")) | ||
| .WithTracing(tracing => tracing | ||
| .AddAspNetCoreInstrumentation() | ||
| .SetSampler(new AlwaysOnSampler()) |
There was a problem hiding this comment.
Maybe we should add AddSource("OpenFeature") to collect OpenFeature activity?
* Ensure OpenFeature traces are recorded in sample app * Ensure ActivitySource is disposed of Signed-off-by: Kyle Julian <38759683+kylejuliandev@users.noreply.github.com>
* Add check on IsAllDataRequested to save on allocations on the activity span Signed-off-by: Kyle Julian <38759683+kylejuliandev@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces OpenTelemetry instrumentation to the OpenFeature .NET SDK by adding a new OpenFeatureActivitySource and integrating activity tracking into the OpenFeatureClient evaluation logic. The changes include mapping evaluation results and errors to standard semantic conventions and adding comprehensive tests. Review feedback suggests aligning specific attribute names with OpenTelemetry standards (specifically provider_name and variant), normalizing reason strings to lowercase, and removing a redundant Dispose() call on an activity already managed by a using statement.
Signed-off-by: Kyle Julian <38759683+kylejuliandev@users.noreply.github.com>
| activity?.SetStatus(ActivityStatusCode.Error); | ||
|
|
||
| if (activity?.IsAllDataRequested == true) | ||
| { | ||
| activity.AddTag("error.type", OpenFeatureActivitySource.GetFlagEvaluationErrorDescription(evaluation.ErrorType)); | ||
| activity.SetTag(OpenFeatureActivitySource.FeatureFlagErrorMessageName, evaluation.ErrorMessage); | ||
| } |
There was a problem hiding this comment.
seems there's some duplication, maybe we could add one method to avoid duplicates
private static void AttachActivityTags(Activity? activity, FlagEvaluationDetails<T>? evaluation, Exception? exception = null)
{
if (activity is null) return;
if (elaluation?.ErrorType != ErrorType.None)
activity.SetStatus(ActivityStatusCode.Error);
if (activity.IsAllDataRequested != true) return;
if (elaluation is null)
{
// ...
return;
}
activity.SetTag(OpenFeatureActivitySource.FeatureFlagReasonName, OpenFeatureActivitySource.GetFlagEvaluationReasonDescription(evaluation.Reason));
if (elaluation?.ErrorType == ErrorType.None)
{
activity.SetTag(OpenFeatureActivitySource.FeatureFlagValueName, evaluation.Value);
activity.SetTag(OpenFeatureActivitySource.FeatureFlagVariantName, evaluation.Variant);
}
else
{
activity.AddTag("error.type", OpenFeatureActivitySource.GetFlagEvaluationErrorDescription(evaluation.ErrorType));
activity.SetTag(OpenFeatureActivitySource.FeatureFlagErrorMessageName, evaluation.ErrorMessage);
}
}There was a problem hiding this comment.
Actually with all IsAllDataRequested I wonder if makes sense to have an internal extension method to have the AddTag and SetTag. Like AddTagIfRequested or SetTagIfRequested. It would make the code cleaner IMO.
What do you think @WeihanLi and @kylejuliandev?
There was a problem hiding this comment.
I think I prefer the extension method approach. It keeps activity enrichment close to the code but simplifies the repeated if IsAllDataRequested checks
There was a problem hiding this comment.
think it's more cleaner to introduce extension method for AddTagIfRequested/SetTagIfRequested, but it may cause some duplicated not null and IsAllDataRequested checks, maybe it should be avoid for a hotpath.
While maybe we could evaluate this further if someone complains about this.
Not at this point. Until OTEL formally deprecates |
Signed-off-by: Kyle Julian <38759683+kylejuliandev@users.noreply.github.com>
Signed-off-by: Kyle Julian <38759683+kylejuliandev@users.noreply.github.com>
Signed-off-by: Kyle Julian <38759683+kylejuliandev@users.noreply.github.com>
This PR
Add trace when evaluating feature flags. This is different from the
TraceEnricherHookwhich enriches the current span or trace with an event, instead this more closely follows the semantic conventions: https://opentelemetry.io/docs/specs/semconv/feature-flags/feature-flags-events/. Also add OpenTelemetry logging to the sample application and ensure traces are always sampled.Success tracwe
Error trace
Related Issues
Fixes #595
Fixes #541
Notes
Follow-up Tasks
How to test