refactor: use attribute.KeyValue instead of map
#418
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,96 +5,64 @@ import ( | |
| "strings" | ||
|
|
||
| "github.com/open-feature/go-sdk/openfeature" | ||
| "go.opentelemetry.io/otel/attribute" | ||
| semconv "go.opentelemetry.io/otel/semconv/v1.34.0" | ||
| ) | ||
|
|
||
| // EventName is the name of the feature flag evaluation event. | ||
| const EventName string = "feature_flag.evaluation" | ||
|
There was a problem hiding this comment. this is what you & Michael were asking when it would be added to There was a problem hiding this comment. Yes, it doesn't currently exist in the latest semconv 1.34.0, which is why this const is needed for now |
||
|
|
||
| const ( | ||
| flagMetaContextIDKey string = "contextId" | ||
| flagMetaFlagSetIDKey string = "flagSetId" | ||
| flagMetaVersionKey string = "version" | ||
| ) | ||
|
|
||
| // EventAttributes returns a slice of OpenTelemetry attributes that can be used to create an event for a feature flag evaluation. | ||
| // It is intended to be used in the `Finally` stage of a [openfeature.Hook]. | ||
| func EventAttributes(hookContext openfeature.HookContext, details openfeature.InterfaceEvaluationDetails) []attribute.KeyValue { | ||
|
There was a problem hiding this comment. Since this is a new function, should we keep the old one with a deprecation warning? I don't want technical debt to pile up, but this seems like an easy way to avoid abruptly adding a breaking change. There was a problem hiding this comment. We could branch our If we want to implement this in contribs instead, to avoid the OTel dep (which would be my preference) I'd recommend we just cut the WDYT @beeme1mr @sahidvelji ? |
||
| attributes := []attribute.KeyValue{ | ||
| semconv.FeatureFlagKey(hookContext.FlagKey()), | ||
| semconv.FeatureFlagProviderName(hookContext.ProviderMetadata().Name), | ||
| } | ||
|
|
||
| reason := strings.ToLower(string(openfeature.UnknownReason)) | ||
| if details.Reason != "" { | ||
| reason = strings.ToLower(string(details.Reason)) | ||
| } | ||
| attributes = append(attributes, semconv.FeatureFlagResultReasonKey.String(reason)) | ||
|
|
||
| if details.Variant != "" { | ||
| attributes = append(attributes, semconv.FeatureFlagResultVariant(details.Variant)) | ||
| } | ||
|
|
||
| contextID := hookContext.EvaluationContext().TargetingKey() | ||
| if flagMetaContextID, ok := details.FlagMetadata[flagMetaContextIDKey].(string); ok { | ||
| contextID = flagMetaContextID | ||
| } | ||
| attributes = append(attributes, semconv.FeatureFlagContextID(contextID)) | ||
|
|
||
| if setID, ok := details.FlagMetadata[flagMetaFlagSetIDKey].(string); ok { | ||
| attributes = append(attributes, semconv.FeatureFlagSetID(setID)) | ||
| } | ||
|
|
||
| if version, ok := details.FlagMetadata[flagMetaVersionKey].(string); ok { | ||
| attributes = append(attributes, semconv.FeatureFlagVersion(version)) | ||
| } | ||
|
|
||
| if details.Reason != openfeature.ErrorReason { | ||
| return attributes | ||
| } | ||
|
|
||
| errorType := strings.ToLower(string(openfeature.GeneralCode)) | ||
| if details.ErrorCode != "" { | ||
| errorType = strings.ToLower(string(details.ErrorCode)) | ||
| } | ||
| attributes = append(attributes, semconv.ErrorTypeKey.String(errorType)) | ||
|
|
||
| if details.ErrorMessage != "" { | ||
| attributes = append(attributes, semconv.ErrorMessage(details.ErrorMessage)) | ||
| } | ||
|
|
||
| return attributes | ||
| } | ||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally we like to keep our SDK free of non-test dependencies. Of course, we can make exceptions for some cases, and OTel could be such an exception, but I think I'd advocate for implementing this in the contrib as you alluded to in slack.
I think if we are going to implement it in the SDK directly, we should avoid this dependency. The only SDK that includes OTel bits directly is .NET, but that's a bit of a different case since recent .NET runtimes actually include OTel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@beeme1mr @sahidvelji my main question is, how realistic is it for an org to reject the use of OpenFeature because of this dep? Is OTel "controversial" for some orgs? Might a big org that offers telemetry products, which would otherwise adopt OpenFeature, decide not to just because of this dep?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So Go dependency "tree shaking" happens at the package level. So unless a user imports the
telemetrypackage, they will not get the OTel dependency. But again, given that the OTel hooks live in contrib instead of the SDK, I think it might make sense for this package to also live in contrib instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be fine given how it's used.