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

Feature Flag evaluation event #1440

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

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Sep 30, 2024

First draft of feature_flag.evaluation event from the feature flag semantic conventions working group. Based on proposal here: https://docs.google.com/document/d/1sBDD-uifx-Qy0NxtwRcrRAQ48sHQS9dTp6tpArQsoZg/edit

Summary of changes:

Copy link

@askpt askpt left a comment

Choose a reason for hiding this comment

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

I added a couple of comments; otherwise, it looks good.

model/feature-flag/registry.yaml Outdated Show resolved Hide resolved
model/feature-flag/registry.yaml Show resolved Hide resolved
@dyladan dyladan marked this pull request as ready for review October 28, 2024 13:23
@dyladan dyladan requested review from tigrannajaryan and a team as code owners October 28, 2024 13:23
model/feature-flag/registry.yaml Outdated Show resolved Hide resolved
model/feature-flag/registry.yaml Outdated Show resolved Hide resolved
model/feature-flag/logs.yaml Outdated Show resolved Hide resolved
@jsuereth
Copy link
Contributor

FYI - you need to clean up the event group's id:

Diagnostic report:

Violation: Event id 'feature_flag.evaluation' is invalid. Event id must follow 'event.{name}' pattern and match 'event.feature_flag.evaluation'
  - Category         : yaml_schema_violation
  - Type             : semconv_attribute
  - SemConv group    : feature_flag.evaluation
  - SemConv attribute: feature_flag.evaluation
  - Provenance: /home/weaver/source/feature-flag/logs.yaml  

@dyladan
Copy link
Member Author

dyladan commented Nov 12, 2024

@lmolkova I took the suggestion to use error.type but there isn't an equivalent error.message are we ok keeping the current feature_flag.evaluation.error.message or is there another name we should take for that one?

@dyladan
Copy link
Member Author

dyladan commented Nov 12, 2024

@jsuereth like this? 43cdfd4

Copy link
Contributor

@lmolkova lmolkova left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@joaopgrassi joaopgrassi left a comment

Choose a reason for hiding this comment

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

Please take a look and also, could you please update main/schema-next.yaml (list renamed attributes there)?

This is pending still.

@dyladan
Copy link
Member Author

dyladan commented Nov 13, 2024

Please take a look and also, could you please update main/schema-next.yaml (list renamed attributes there)?

This is pending still.

Added feature_flag.provider_name -> feature_flag.system rename.

There are a couple changes I'm not sure how to make there (or if it's possible).

  1. We removed the span semconv entirely in favor of events
  2. We renamed the event from feature_flag to event.feature_flag.evaluation

@dyladan
Copy link
Member Author

dyladan commented Nov 13, 2024

Please take a look and also, could you please update main/schema-next.yaml (list renamed attributes there)?

This is pending still.

Added feature_flag.provider_name -> feature_flag.system rename.

There are a couple changes I'm not sure how to make there (or if it's possible).

  1. We removed the span semconv entirely in favor of events
  2. We renamed the event from feature_flag to event.feature_flag.evaluation

According to https://opentelemetry.io/docs/specs/otel/schemas/file_format_v1.1.0 it looks like you can rename span_events but not top-level events? Additionally, it's still not clear at all to me how to handle the differences events/logs/span_events. Previously we had a file named log which had an event name and a file named span which had a span event. Now we only have an event. How do we map that in the schema? And if we want to keep span events, do we need multiple files or is it possible to have a single event definition that works for both?

Copy link
Contributor

@lmolkova lmolkova left a comment

Choose a reason for hiding this comment

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

There are a couple changes I'm not sure how to make there (or if it's possible).

We removed the span semconv entirely in favor of events
We renamed the event from feature_flag to event.feature_flag.evaluation

changelog entry is the best we can do today. Added some suggestions

.chloggen/1440.yaml Outdated Show resolved Hide resolved
.chloggen/1440.yaml Outdated Show resolved Hide resolved
Co-authored-by: Liudmila Molkova <[email protected]>
@jsuereth
Copy link
Contributor

@dyladan This Looks good to merge, assuming the conversations are resolved. The yamllint error - let me know if we need to update the settings. We may be hitting limitations in YAML event definitions and "max line length" rules...

@dyladan
Copy link
Member Author

dyladan commented Nov 14, 2024

@dyladan This Looks good to merge, assuming the conversations are resolved. The yamllint error - let me know if we need to update the settings. We may be hitting limitations in YAML event definitions and "max line length" rules...

I wish the yaml lint would tell me which line is failing. I can't find where the error is actually caused. I think it's likely the table that I inlined for the error values? But that's only 197 characters

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ready to be Merged
Development

Successfully merging this pull request may close these issues.