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 appendix D (observability) #287

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

toddbaert
Copy link
Member

  • adds small appendix section to spec that defines how particular SDK fields are mapped to the recently merge OTel semantics
  • this allows providers and hooks to export OTel data consistently for easy interop
  • adds a couple supporting glossary liks

@toddbaert toddbaert force-pushed the feat/appendix-d-observabilty branch from 8dd27f0 to fcbbf2a Compare December 16, 2024 19:20
specification/appendix-d-observability.md Outdated Show resolved Hide resolved
specification/appendix-d-observability.md Outdated Show resolved Hide resolved

| Provider Metadata Field | Log Record Attribute | Notes |
| ----------------------- | ---------------------------- | ------------------------------------------------------------------------------------------------ |
| `name` | `feature_flag.provider_name` | The name of the provider as defined in the `provider metadata`, available in the `hook context`. |
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should add a note that it's recommended to keep provider names consistent across provider implementations.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it's worth mentioning. I guess providers can decide for themselves if they want the name to be consistent.

Copy link
Member

Choose a reason for hiding this comment

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

I think so too @toddbaert, it might be interesting for some consumers to see the provider implementations instead of a common name. But I guess @beeme1mr's case is the more common.

Copy link
Member

Choose a reason for hiding this comment

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

I'm also with @toddbaert regarding a provider name. But I can imagine your point @beeme1mr of having an identifiying property for post processing. Maybe there should also be a provider_id property we could propose in otel?

@toddbaert toddbaert force-pushed the feat/appendix-d-observabilty branch from fcbbf2a to 2d44c6d Compare December 16, 2024 19:43
| ------------------------ | --------------------------------------- | ---------------------------------------------------- |
| `flag key` | `feature_flag.key` | See: [flag key](./glossary.md#flag-key) |
| `error code` | `error.type` | See: [error code](./types.md#error-code) |
| `variant` | `feature_flag.variant` | See: [variant](./glossary.md#variant) |
Copy link
Member Author

Choose a reason for hiding this comment

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

There's questions about how exactly to represent value (important if provider cannot supply variant):

  • is it represented in the body?
  • can it be of any type?

Copy link
Member

Choose a reason for hiding this comment

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

is it represented in the body?

Do you mean the body of the log record?

The spec says:

value: If and only if feature flag provider does not supply variant or equivalent concept. 
Otherwise, value should be treated as opt-in.

And the type is undefined there as there is no any type in OTEL.
Recently there was also opened a PR to add the value_type to the event: open-telemetry/semantic-conventions#1689

@toddbaert
Copy link
Member Author

I've marked as draft for now since theres some ongoing conversations in OTel about how to represent certain types of event data: open-telemetry/semantic-conventions#1651 (comment)

Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[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.

This looks good to me, we might consider the PR I mentioned regarding the value_type, but I think we could also start with the value as any type and add the type hint from the PR later as it does not change the any type.

| ------------------------ | --------------------------------------- | ---------------------------------------------------- |
| `flag key` | `feature_flag.key` | See: [flag key](./glossary.md#flag-key) |
| `error code` | `error.type` | See: [error code](./types.md#error-code) |
| `variant` | `feature_flag.variant` | See: [variant](./glossary.md#variant) |
Copy link
Member

Choose a reason for hiding this comment

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

is it represented in the body?

Do you mean the body of the log record?

The spec says:

value: If and only if feature flag provider does not supply variant or equivalent concept. 
Otherwise, value should be treated as opt-in.

And the type is undefined there as there is no any type in OTEL.
Recently there was also opened a PR to add the value_type to the event: open-telemetry/semantic-conventions#1689


| Provider Metadata Field | Log Record Attribute | Notes |
| ----------------------- | ---------------------------- | ------------------------------------------------------------------------------------------------ |
| `name` | `feature_flag.provider_name` | The name of the provider as defined in the `provider metadata`, available in the `hook context`. |
Copy link
Member

Choose a reason for hiding this comment

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

I think so too @toddbaert, it might be interesting for some consumers to see the provider implementations instead of a common name. But I guess @beeme1mr's case is the more common.

| `flag key` | `feature_flag.key` | See: [flag key](./glossary.md#flag-key) |
| `error code` | `error.type` | See: [error code](./types.md#error-code) |
| `variant` | `feature_flag.variant` | See: [variant](./glossary.md#variant) |
| `error message` | `feature_flag.evaluation.error.message` | An error message associated with a failed evaluation |
Copy link
Member

Choose a reason for hiding this comment

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

Should we here also make clear that it is not recommended to not repat error.type here, like it's stated in the otel semcon?

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.

4 participants