Skip to content
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

feat: add Otel util func #325

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

Conversation

bbland1
Copy link
Contributor

@bbland1 bbland1 commented Feb 19, 2025

This PR

  • it should be equivalent to the JS implementation
  • added the logic to return the evaluation flag event

Related Issues

Fixes #322

How to test

  • Unit, Fuzz & E2E tests all pass locally

@bbland1 bbland1 marked this pull request as ready for review February 19, 2025 21:11
@bbland1 bbland1 requested a review from a team as a code owner February 19, 2025 21:11
Copy link

codecov bot commented Feb 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.13%. Comparing base (de5f7c3) to head (78afe2b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #325      +/-   ##
==========================================
+ Coverage   86.78%   88.13%   +1.35%     
==========================================
  Files          13       14       +1     
  Lines        1362     1408      +46     
==========================================
+ Hits         1182     1241      +59     
+ Misses        156      143      -13     
  Partials       24       24              
Flag Coverage Δ
e2e 88.13% <100.00%> (+1.35%) ⬆️
unit 88.13% <100.00%> (+1.35%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

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.

This is great, thank you!

…o set what is being set by the if-else

Signed-off-by: bbland1 <[email protected]>
Copy link
Member

@lukas-reining lukas-reining 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 to me! Thanks!

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.

I had a few thoughts after rereviewing this. I'd consider them all optional but worth consider. Thanks!

Copy link
Member

@liran2000 liran2000 left a comment

Choose a reason for hiding this comment

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

Some const values were not used, left comments on what to replace.

no linter caught this.


contextID, exists := details.EvaluationDetails.ResolutionDetail.FlagMetadata[TelemetryFlagMetaContextId]
if exists && contextID != "" {
attributes[TelemetryFlagMetaContextId] = contextID
Copy link
Member

Choose a reason for hiding this comment

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

replace to TelemetryContextID ?

if exists && contextID != "" {
attributes[TelemetryFlagMetaContextId] = contextID
} else {
attributes[TelemetryFlagMetaContextId] = hookContext.EvaluationContext().TargetingKey()
Copy link
Member

Choose a reason for hiding this comment

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

replace to TelemetryContextID ?


setID, exists := details.EvaluationDetails.ResolutionDetail.FlagMetadata[TelemetryFlagMetaFlagSetId]
if exists {
attributes[TelemetryFlagMetaFlagSetId] = setID
Copy link
Member

Choose a reason for hiding this comment

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

replace to TelemetryFlagSetID ?


version, exists := details.EvaluationDetails.ResolutionDetail.FlagMetadata[TelemetryFlagMetaVersion]
if exists {
attributes[TelemetryFlagMetaVersion] = version
Copy link
Member

Choose a reason for hiding this comment

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

replace to TelemetryVersion ?

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
5 participants