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

Support expanded OpenTelemetry semantic attributes #281

Closed
beeme1mr opened this issue Nov 21, 2024 · 5 comments · Fixed by #280
Closed

Support expanded OpenTelemetry semantic attributes #281

beeme1mr opened this issue Nov 21, 2024 · 5 comments · Fixed by #280
Assignees

Comments

@beeme1mr
Copy link
Member

Overview

OpenFeature recently partnered with OpenTelemetry to define the next version of the feature flag semantic convention. This significantly expanded the number of named attributes to better support more sophisticated use cases. Most of the attributes are readily available in OpenFeature hooks. However, some provider-specific attributes like feature_flag.set.id and feature_flag.version are not easily accessible at the moment. The OpenFeature specification must be adjusted to fully support the new OTel semantics.

Proposal

Modify the finally stage in hooks to include the evaluation details. The evaluation details should match what's returned to the application developer. This would allow providers to set defined flag metadata that would then be available in a hook. If this proposal is accepted, it may also be worth explicitly defining some well-known attributes to make it easier for provider developers.

Consideration

This proposal is a breaking change but should have little to no impact on end users. The finally stage had limited value before and was primarily used to end spans or request-based metrics.

Another consideration is that providers must return flag metadata in their responses so that they're available in hooks. Providers should also avoid throwing errors and instead return responses with the REASON set to ERROR so that flag metadata can be included in the response.

@kinyoklion
Copy link
Member

kinyoklion commented Nov 25, 2024

@beeme1mr We should also track some update to allow for provider specific implementation of feature_flag.context.id. While we can provide default support that uses the targeting key, that will not work in all cases. The hook ordering means that the provider hooks will run after telemetry, so it would be unable to modify the context as an alternative.

For example in LaunchDarkly if we had the following context shape:

{
  user: {key: 'user-key'},
  organization: {key: 'my-organization'}
}

We need a way for the feature_flag.context.id to be organization:my-organization:user:user-key. Though in the basic single-context case we could utilize the targeting key.

The only way I have really thought of so far would be an optional provider method. Something like getContextId which is provided the evaluation context and returns an ID. The default implementation would just return the targetingKey (when the targetingKey is provided).

Thank you,
Ryan

@beeme1mr
Copy link
Member Author

Hey @kinyoklion, perhaps the provider could set the context.id as flag metadata. That would give the provider full control over how that value is calculated. The OTel hook could check for that value first and then possibly use targetingKey as a fallback.

For the next steps, I'd like to extend flag metadata to have some named properties like context.id, flag set id, and flag set version (exact names TBD). After that, I'll try to specify the hook behavior so that it can be consistently implemented.

Will that work for your requirements?

@beeme1mr beeme1mr self-assigned this Nov 26, 2024
@kinyoklion
Copy link
Member

@beeme1mr It may work for this specific situation, in that it will work with events/logs which only harvest data after evaluation.

I would be concerned with how we implement context.id in order to not conflict with a context attribute of "id". For example if someone currently has a context attribute of "id" and their targeting depending on "id", then putting "id" in would cause evaluation problems.

It doesn't work with anything you want to be able to correlate with before evaluation, which makes it feel a bit fragile (supporting this specific use-case, but not really the wider potential). If we had the hook-data as well, then someone could use that to add correlation data to something after an evaluation. (Collecting data in earlier stages, pulling out of hook-data in a later state and adding the context id.)

@beeme1mr
Copy link
Member Author

beeme1mr commented Dec 2, 2024

Hey @kinyoklion, could you please elaborate on how the hook-data proposal would help? I thought the hook data was scoped to each hook.

@kinyoklion
Copy link
Member

@beeme1mr hook-data is scoped per hook and per-evaluation, and allows data to flow between stages. So if you needed to correlate something before evaluation to something after evaluation, then you would add it to your hook-data before evaluation, and then get the context.id after the evaluation. You would need to do that in whatever hook cared about that correlation. Not as convenient as the context.id just being there before the hooks start getting invoked though. You couldn't actually act on it until later.

That said the only real use-case I have had for it is for making spans, and in that case you always need to do something later anyway. I think that point-in-time related operations are really the only things that need that relationship because you don't really have any extra data before evaluation. So maybe it isn't very important and the larger concern is not conflicting with existing attributes.

@beeme1mr beeme1mr linked a pull request Dec 11, 2024 that will close this issue
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 a pull request may close this issue.

2 participants