Skip to content

feat: Add OTEL compatible telemetry object builder #397

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

askpt
Copy link
Member

@askpt askpt commented Mar 7, 2025

This PR

This pull request introduces a new feature for telemetry in the OpenFeature library, specifically focusing on evaluation events for feature flags. The main changes include the addition of new classes and constants to support the creation and handling of these events, as well as tests to ensure the functionality works as expected.

New Features for Telemetry:

Constants for Telemetry:

Tests for Evaluation Event Builder:

Related Issues

Fixes #381

@askpt askpt requested a review from a team as a code owner March 7, 2025 14:00
@askpt askpt linked an issue Mar 7, 2025 that may be closed by this pull request
@askpt askpt requested a review from Copilot March 7, 2025 14:00
@askpt askpt changed the title Add OTEL compatible telemetry object builder feat: Add OTEL compatible telemetry object builder Mar 7, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR adds OTEL-compatible telemetry support to the OpenFeature library by introducing new classes, constants, and tests for building evaluation events used in feature flag evaluations.

  • Added telemetry constants and flag metadata classes.
  • Implemented an evaluation event builder to construct OTEL-compliant evaluation events.
  • Included unit tests to verify the correct construction of evaluation events.

Reviewed Changes

File Description
src/OpenFeature/Telemetry/TelemetryConstants.cs Introduces constants for telemetry event attributes.
src/OpenFeature/Telemetry/EvaluationEventBuilder.cs Implements the builder for evaluation events.
test/OpenFeature.Tests/Telemetry/EvaluationEventBuilderTests.cs Adds tests to validate evaluation event building, including error handling and missing variants.
src/OpenFeature/Telemetry/TelemetryFlagMetadata.cs Defines well-known metadata keys for telemetry.
src/OpenFeature/Telemetry/TelemetryEvaluationData.cs Adds evaluation data constants for telemetry events.
src/OpenFeature/Telemetry/EvaluationEvent.cs Provides the EvaluationEvent class used to represent an event.

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

src/OpenFeature/Telemetry/EvaluationEventBuilder.cs:28

  • The reference 'Reason.Unknown' is undefined in this context; please ensure that the appropriate constant or enum is referenced instead.
attributes[TelemetryConstants.Reason] = !string.IsNullOrWhiteSpace(details.Reason) ? details.Reason?.ToLowerInvariant() : Reason.Unknown;

test/OpenFeature.Tests/Telemetry/EvaluationEventBuilderTests.cs:2

  • The using directive 'using OpenFeature.Constant;' in the test file does not match the namespace of TelemetryConstants. Please update it to 'using OpenFeature.Telemetry;' to ensure consistency.
using OpenFeature.Constant;

@askpt askpt enabled auto-merge March 7, 2025 14:03
Copy link

codecov bot commented Mar 7, 2025

Codecov Report

Attention: Patch coverage is 80.64516% with 6 lines in your changes missing coverage. Please review.

Project coverage is 86.36%. Comparing base (0821b3b) to head (6b48487).

Files with missing lines Patch % Lines
...rc/OpenFeature/Telemetry/EvaluationEventBuilder.cs 72.72% 0 Missing and 6 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #397      +/-   ##
==========================================
- Coverage   86.47%   86.36%   -0.11%     
==========================================
  Files          42       44       +2     
  Lines        1671     1702      +31     
  Branches      177      185       +8     
==========================================
+ Hits         1445     1470      +25     
  Misses        187      187              
- Partials       39       45       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…e tests for missing flag metadata

Signed-off-by: André Silva <[email protected]>
@askpt askpt requested a review from Copilot March 7, 2025 14:33
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR adds support for OTEL-compatible telemetry events for feature flags in the OpenFeature library. The changes include the creation of new classes for evaluation events and their builders, the definition of related constants, and comprehensive tests for the new functionality.

  • Added telemetry constants and flag metadata definitions.
  • Introduced the EvaluationEvent and EvaluationEventBuilder classes.
  • Added unit tests to verify the behavior of evaluation event creation.

Reviewed Changes

File Description
src/OpenFeature/Telemetry/TelemetryConstants.cs Defines constants for telemetry attribute keys.
src/OpenFeature/Telemetry/EvaluationEventBuilder.cs Provides a builder for creating evaluation events; review notes below.
test/OpenFeature.Tests/Telemetry/EvaluationEventBuilderTests.cs Contains tests that verify evaluation event creation behavior.
src/OpenFeature/Telemetry/TelemetryEvaluationData.cs Defines additional telemetry event data constants.
src/OpenFeature/Telemetry/TelemetryFlagMetadata.cs Specifies well-known flag metadata attribute keys.
src/OpenFeature/Telemetry/EvaluationEvent.cs Implements the evaluation event model with attributes and body.

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

src/OpenFeature/Telemetry/EvaluationEventBuilder.cs:2

  • The using directive 'OpenFeature.Constant' appears inconsistent with the location of TelemetryConstants, which is in the 'OpenFeature.Telemetry' namespace. Consider updating or removing this directive to avoid potential namespace conflicts.
using OpenFeature.Constant;

src/OpenFeature/Telemetry/EvaluationEventBuilder.cs:31

  • The use of 'Reason.Unknown' appears to reference an undefined member. Consider defining a constant (e.g., TelemetryConstants.DefaultReason or similar) or replacing it with an appropriate default value such as "unknown".
attributes[TelemetryConstants.Reason] = !string.IsNullOrWhiteSpace(details.Reason) ? details.Reason?.ToLowerInvariant() : Reason.Unknown;

…er and add corresponding unit tests

Signed-off-by: André Silva <[email protected]>
@askpt askpt force-pushed the askpt/381-add-opentelemetry-compatible-telemetry branch from e016fcf to 2d81ada Compare March 7, 2025 14:41
@toddbaert toddbaert requested a review from beeme1mr March 7, 2025 18:43
Copy link
Member

@beeme1mr beeme1mr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but please make sure error code and reason is lowercase.

var evaluationEvent = EvaluationEventBuilder.Build(hookContext, details);

// Assert
Assert.Equal("General", evaluationEvent.Attributes[TelemetryConstants.ErrorCode]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be lowercase.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reminding me. Should be fixed with 0d1a869

@askpt askpt requested a review from beeme1mr March 11, 2025 16:27
Copy link
Member

@beeme1mr beeme1mr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI @askpt, I found out yesterday that the OTel team has required some changes to the semcon. Let's wait on merging until we have further clarity.

@askpt
Copy link
Member Author

askpt commented Apr 17, 2025

FYI @askpt, I found out yesterday that the OTel team has required some changes to the semcon. Let's wait on merging until we have further clarity.

@beeme1mr This PR is now updated with the latest semcov conventions. When we get the spec updated, we can merge this

@askpt askpt mentioned this pull request Apr 23, 2025
@askpt askpt disabled auto-merge April 24, 2025 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add OpenTelemetry-Compatible Telemetry Utility Function
3 participants