-
Notifications
You must be signed in to change notification settings - Fork 0
feat: update tracing hook to latest semantic conventions #12
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
Merged
Conversation
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
- Rename feature_flag.context.key to feature_flag.context.id - Fix feature_flag.provider_name to feature_flag.provider.name (add missing dot) - Rename feature_flag.variant to feature_flag.result.value - Add include_value configuration option (keep include_variant as deprecated) - Add feature_flag.result.reason.inExperiment attribute - Add feature_flag.result.variationIndex attribute - Add feature_flag.set.id attribute with environment_id configuration support - Update all tests to use new attribute names - Add tests for new attributes Follows OpenTelemetry semantic conventions specification: https://raw.githubusercontent.com/launchdarkly/sdk-specs/refs/heads/main/specs/OTEL-openteletry-integration/README.md Reference implementations: - Go SDK: launchdarkly/go-server-sdk#292 - .NET SDK: launchdarkly/dotnet-core#148 Co-Authored-By: Vadim Korolik <[email protected]>
Contributor
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
The validate_environment_id method needs @logger to be initialized before it can log warnings about invalid environment_id values. Fixed by reordering initialization in TracingHookOptions constructor. Co-Authored-By: Vadim Korolik <[email protected]>
The environment_id and inExperiment/variationIndex test contexts are nested inside 'with add_spans' and need 4-space indentation. Co-Authored-By: Vadim Korolik <[email protected]>
Both environment_id and inExperiment contexts are nested inside 'with add_spans' context and need consistent 4-space indentation. Also removed trailing whitespace from blank lines. Co-Authored-By: Vadim Korolik <[email protected]>
keelerm84
approved these changes
Oct 17, 2025
- Use inline private modifier for validate_environment_id - Refactor to guarded return style for better readability Co-Authored-By: Vadim Korolik <[email protected]>
Contributor
|
Thanks for the code review @keelerm84! I've applied both style suggestions:
All CI checks are passing with these changes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Updates the Ruby OpenTelemetry tracing hook to comply with the latest OpenTelemetry semantic conventions specification for feature flag instrumentation. This brings the Ruby SDK in line with other LaunchDarkly SDKs (Java, Go, .NET).
Link to Devin run: https://app.devin.ai/sessions/00115de67e494cc999f5c247414cd9b1
Requested by: @Vadman97
Changes
Attribute Name Updates (Breaking for Observability Consumers)
feature_flag.context.key→feature_flag.context.idfeature_flag.provider_name→feature_flag.provider.namefeature_flag.variant→feature_flag.result.valueNew Configuration Options
include_value: Replaces deprecatedinclude_variantoption. Controls whether flag values are included in telemetry.environment_id: Optional string parameter that addsfeature_flag.set.idattribute. Validates non-empty strings and logs warnings for invalid input.New Optional Attributes
feature_flag.result.reason.inExperiment: Set totruewhen evaluation is part of an experimentfeature_flag.result.variationIndex: Includes variation index when availableBackward Compatibility
include_variantoption continues to work via fallback logic:opts.fetch(:include_value, opts.fetch(:include_variant, false))Bug Fixes
validate_environment_idwas called before@loggerwas initialized, causingNoMethodErroron Ruby 3.0/3.1Testing
include_valueconfigurationinclude_variantbackward compatibilityenvironment_idconfiguration (valid and invalid cases)inExperimentandvariationIndexattributesReview Checklist
Requirements
Test logic for
inExperiment(line 221 in spec): The test "includes inExperiment when evaluation is part of experiment" verifies the attribute is NOT present (.to be false). Please verify this is correct behavior - is the test setup actually creating an experiment scenario, or is this testing the default case?Backward compatibility: Verify the nested fallback pattern
opts.fetch(:include_value, opts.fetch(:include_variant, false))works correctly for all combinations of options.Initialization order: The constructor now sets
@loggerbefore callingvalidate_environment_id. Verify this doesn't break any assumptions about initialization sequence.Environment validation: The
environment_idvalidation only checks for non-empty strings. Confirm this matches the spec requirements (no format/character restrictions needed?).Related Issues
Additional Context
ruby-server-sdk-otelpackage; the mainruby-server-sdkpackage does not require updates.