fix: conform feature_flag span event to the OTEL spec

ldotel/testing/test_tracing.py

@@ -1,3 +1,6 @@
+from dataclasses import dataclass
+from typing import Any, Optional
+
 import pytest
 from ldclient import Config, Context, LDClient
 from ldclient.evaluation import EvaluationDetail
@@ -13,6 +16,31 @@
 from ldotel.tracing import Hook, HookOptions
 
 
+@dataclass
+class _SeriesContextWithoutEnvironmentId:
+    """
+    The shape of ``EvaluationSeriesContext`` in SDK versions which do not
+    report the environment ID to hooks.
+    """
+
+    key: str
+    context: Context
+    default_value: Any
+    method: str
+
+
+def _series_context(environment_id: Optional[str]) -> EvaluationSeriesContext:
+    series_context = EvaluationSeriesContext(
+        key='boolean',
+        context=Context.create('org-key', 'org'),
+        default_value=False,
+        method='variation',
+    )
+    setattr(series_context, 'environment_id', environment_id)
+
+    return series_context
+
+
 @pytest.fixture
 def td() -> TestData:
     td = TestData.data_source()
@@ -63,7 +91,7 @@ def test_records_basic_span_event(self, client: LDClient, exporter: SpanExporter
         assert event.attributes['feature_flag.key'] == 'boolean'
         assert event.attributes['feature_flag.provider.name'] == 'LaunchDarkly'
         assert event.attributes['feature_flag.context.id'] == 'org:org-key'
-        assert event.attributes['feature_flag.result.variationIndex'] == '0'
+        assert event.attributes['feature_flag.result.variationIndex'] == 0
         assert 'feature_flag.result.value' not in event.attributes
         assert 'feature_flag.result.reason.inExperiment' not in event.attributes
 
@@ -81,7 +109,7 @@ def test_can_include_variant(self, client: LDClient, exporter: SpanExporter, tra
         assert event.attributes['feature_flag.key'] == 'boolean'
         assert event.attributes['feature_flag.provider.name'] == 'LaunchDarkly'
         assert event.attributes['feature_flag.context.id'] == 'org:org-key'
-        assert event.attributes['feature_flag.result.variationIndex'] == '0'
+        assert event.attributes['feature_flag.result.variationIndex'] == 0
         assert event.attributes['feature_flag.result.value'] == 'true'
         assert 'feature_flag.result.reason.inExperiment' not in event.attributes
 
@@ -112,7 +140,7 @@ def test_can_include_value_types(self, flag_key, variations, variation_index, ex
         assert event.attributes['feature_flag.key'] == flag_key
         assert event.attributes['feature_flag.provider.name'] == 'LaunchDarkly'
         assert event.attributes['feature_flag.context.id'] == 'org:org-key'
-        assert event.attributes['feature_flag.result.variationIndex'] == str(variation_index)
+        assert event.attributes['feature_flag.result.variationIndex'] == variation_index
         assert event.attributes['feature_flag.result.value'] == json.dumps(expected_value)
         assert 'feature_flag.result.reason.inExperiment' not in event.attributes
 
@@ -146,7 +174,7 @@ def test_add_span_leaves_events_on_top_level_span(self, client: LDClient, export
         assert event.attributes['feature_flag.key'] == 'boolean'
         assert event.attributes['feature_flag.provider.name'] == 'LaunchDarkly'
         assert event.attributes['feature_flag.context.id'] == 'org:org-key'
-        assert event.attributes['feature_flag.result.variationIndex'] == '0'
+        assert event.attributes['feature_flag.result.variationIndex'] == 0
         assert 'feature_flag.result.value' not in event.attributes
         assert 'feature_flag.result.reason.inExperiment' not in event.attributes
 
@@ -174,15 +202,15 @@ def test_hook_makes_its_span_active(self, client: LDClient, exporter: SpanExport
         assert middle.events[0].attributes['feature_flag.key'] == 'boolean'
         assert middle.events[0].attributes['feature_flag.provider.name'] == 'LaunchDarkly'
         assert middle.events[0].attributes['feature_flag.context.id'] == 'org:org-key'
-        assert middle.events[0].attributes['feature_flag.result.variationIndex'] == '0'
+        assert middle.events[0].attributes['feature_flag.result.variationIndex'] == 0
         assert 'feature_flag.result.value' not in middle.events[0].attributes
         assert 'feature_flag.result.reason.inExperiment' not in middle.events[0].attributes
 
         assert top.events[0].name == 'feature_flag'
         assert top.events[0].attributes['feature_flag.key'] == 'boolean'
         assert top.events[0].attributes['feature_flag.provider.name'] == 'LaunchDarkly'
         assert top.events[0].attributes['feature_flag.context.id'] == 'org:org-key'
-        assert top.events[0].attributes['feature_flag.result.variationIndex'] == '0'
+        assert top.events[0].attributes['feature_flag.result.variationIndex'] == 0
         assert 'feature_flag.result.value' not in top.events[0].attributes
         assert 'feature_flag.result.reason.inExperiment' not in top.events[0].attributes
 
@@ -215,8 +243,8 @@ def test_records_in_experiment_attribute(self, exporter: SpanExporter, tracer: T
         assert event.attributes['feature_flag.key'] == 'experiment-flag'
         assert event.attributes['feature_flag.provider.name'] == 'LaunchDarkly'
         assert event.attributes['feature_flag.context.id'] == 'org:org-key'
-        assert event.attributes['feature_flag.result.variationIndex'] == '1'
-        assert event.attributes['feature_flag.result.reason.inExperiment'] == 'true'
+        assert event.attributes['feature_flag.result.variationIndex'] == 1
+        assert event.attributes['feature_flag.result.reason.inExperiment'] is True
         assert 'feature_flag.result.value' not in event.attributes
 
     def test_does_not_include_variation_index_when_none(self, exporter: SpanExporter, tracer: Tracer):
@@ -251,3 +279,110 @@ def test_does_not_include_variation_index_when_none(self, exporter: SpanExporter
         assert 'feature_flag.result.variationIndex' not in event.attributes
         assert 'feature_flag.result.reason.inExperiment' not in event.attributes
         assert 'feature_flag.result.value' not in event.attributes
+
+    def test_records_attributes_with_specified_types(self, exporter: SpanExporter, tracer: Tracer):
+        """
+        The OTEL spec types variationIndex as an int and inExperiment as a
+        boolean. Guard against them regressing to strings, which would break
+        consumers that match on the typed value.
+        """
+        series_context = EvaluationSeriesContext(
+            key='experiment-flag',
+            context=Context.create('org-key', 'org'),
+            default_value=False,
+            method='variation',
+        )
+        detail = EvaluationDetail(value=True, variation_index=1, reason={"inExperiment": True})
+
+        hook = Hook()
+        with tracer.start_as_current_span("test_records_attributes_with_specified_types"):
+            data = hook.before_evaluation(series_context, {})  # type: ignore
+            hook.after_evaluation(series_context, data, detail)  # type: ignore
+
+        event = exporter.get_finished_spans()[0].events[0]  # type: ignore[attr-defined]
+
+        variation_index = event.attributes['feature_flag.result.variationIndex']
+        assert isinstance(variation_index, int) and not isinstance(variation_index, bool)
+        assert variation_index == 1
+
+        in_experiment = event.attributes['feature_flag.result.reason.inExperiment']
+        assert isinstance(in_experiment, bool)
+        assert in_experiment is True
+
+    def test_records_set_id_when_environment_id_configured(self, client: LDClient, exporter: SpanExporter, tracer: Tracer):
+        client.add_hook(Hook(HookOptions(environment_id='my-environment-id')))
+        with tracer.start_as_current_span("test_records_set_id_when_environment_id_configured"):
+            client.variation('boolean', Context.create('org-key', 'org'), False)
+
+        event = exporter.get_finished_spans()[0].events[0]  # type: ignore[attr-defined]
+        assert event.attributes['feature_flag.set.id'] == 'my-environment-id'
+
+    def test_omits_set_id_when_environment_id_not_configured(self, client: LDClient, exporter: SpanExporter, tracer: Tracer):
+        client.add_hook(Hook())
+        with tracer.start_as_current_span("test_omits_set_id_when_environment_id_not_configured"):
+            client.variation('boolean', Context.create('org-key', 'org'), False)
+
+        event = exporter.get_finished_spans()[0].events[0]  # type: ignore[attr-defined]
+        assert 'feature_flag.set.id' not in event.attributes
+
+    def test_records_set_id_from_series_context(self, exporter: SpanExporter, tracer: Tracer):
+        series_context = _series_context(environment_id='series-environment-id')
+
+        hook = Hook()
+        with tracer.start_as_current_span("test_records_set_id_from_series_context"):
+            data = hook.before_evaluation(series_context, {})  # type: ignore
+            hook.after_evaluation(series_context, data, EvaluationDetail(value=True, variation_index=0, reason={}))  # type: ignore
+
+        event = exporter.get_finished_spans()[0].events[0]  # type: ignore[attr-defined]
+        assert event.attributes['feature_flag.set.id'] == 'series-environment-id'
+
+    def test_configured_environment_id_takes_precedence_over_series_context(self, exporter: SpanExporter, tracer: Tracer):
+        series_context = _series_context(environment_id='series-environment-id')
+
+        hook = Hook(HookOptions(environment_id='configured-environment-id'))
+        with tracer.start_as_current_span("test_configured_environment_id_takes_precedence_over_series_context"):
+            data = hook.before_evaluation(series_context, {})  # type: ignore
+            hook.after_evaluation(series_context, data, EvaluationDetail(value=True, variation_index=0, reason={}))  # type: ignore
+
+        event = exporter.get_finished_spans()[0].events[0]  # type: ignore[attr-defined]
+        assert event.attributes['feature_flag.set.id'] == 'configured-environment-id'
+
+    @pytest.mark.parametrize("environment_id", [None, ''])
+    def test_omits_set_id_when_series_context_environment_id_is_unusable(self, environment_id, exporter: SpanExporter, tracer: Tracer):
+        series_context = _series_context(environment_id=environment_id)
+
+        hook = Hook()
+        with tracer.start_as_current_span("test_omits_set_id_when_series_context_environment_id_is_unusable"):
+            data = hook.before_evaluation(series_context, {})  # type: ignore
+            hook.after_evaluation(series_context, data, EvaluationDetail(value=True, variation_index=0, reason={}))  # type: ignore
+
+        event = exporter.get_finished_spans()[0].events[0]  # type: ignore[attr-defined]
+        assert 'feature_flag.set.id' not in event.attributes
+
+    def test_omits_set_id_when_series_context_has_no_environment_id_attribute(self, exporter: SpanExporter, tracer: Tracer):
+        series_context = _SeriesContextWithoutEnvironmentId(
+            key='boolean',
+            context=Context.create('org-key', 'org'),
+            default_value=False,
+            method='variation',
+        )
+
+        hook = Hook()
+        with tracer.start_as_current_span("test_omits_set_id_when_series_context_has_no_environment_id_attribute"):
+            data = hook.before_evaluation(series_context, {})  # type: ignore
+            hook.after_evaluation(series_context, data, EvaluationDetail(value=True, variation_index=0, reason={}))  # type: ignore
+
+        event = exporter.get_finished_spans()[0].events[0]  # type: ignore[attr-defined]
+        assert 'feature_flag.set.id' not in event.attributes
+
+    @pytest.mark.parametrize("environment_id", ['', 0, False, []])
+    def test_ignores_invalid_environment_id(self, environment_id, td: TestData, exporter: SpanExporter, tracer: Tracer):
+        config = Config('sdk-key', update_processor_class=td, send_events=False)
+        client = LDClient(config=config)
+        client.add_hook(Hook(HookOptions(environment_id=environment_id)))
+
+        with tracer.start_as_current_span("test_ignores_invalid_environment_id"):
+            client.variation('boolean', Context.create('org-key', 'org'), False)
+
+        event = exporter.get_finished_spans()[0].events[0]  # type: ignore[attr-defined]
+        assert 'feature_flag.set.id' not in event.attributes

ldotel/tracing.py

@@ -1,6 +1,7 @@
 import json
 import warnings
 from dataclasses import dataclass
+from typing import Dict, Optional
 
 from ldclient.evaluation import EvaluationDetail
 from ldclient.hook import EvaluationSeriesContext
@@ -9,6 +10,7 @@
 from opentelemetry import trace
 from opentelemetry.context import attach, detach
 from opentelemetry.trace import Span, get_current_span, set_span_in_context
+from opentelemetry.util.types import AttributeValue
 
 
 @dataclass
@@ -39,11 +41,32 @@ class HookOptions:
     span events.
     """
 
+    environment_id: Optional[str] = None
+    """
+    If set, then the tracing hook will add the environment ID to span events as
+    the ``feature_flag.set.id`` attribute.
+
+    SDK versions which report the environment ID to hooks do so automatically,
+    so this option is only required for SDK versions which do not. When both
+    are available, this option takes precedence.
+
+    The value must be a non-empty string. Any other value is ignored, which is
+    equivalent to not specifying an environment ID at all.
+    """
+
+
+def _valid_environment_id(environment_id: Optional[str]) -> Optional[str]:
+    if isinstance(environment_id, str) and environment_id != '':
+        return environment_id
+
+    return None
+
 
 class Hook(LDHook):
     def __init__(self, options: HookOptions = HookOptions()):
         self.__tracer = trace.get_tracer_provider().get_tracer("launchdarkly")
         self.__options = options
+        self.__environment_id = _valid_environment_id(options.environment_id)
         if self.__options.include_variant:
             warnings.warn(
                 "The 'include_variant' option is deprecated and will be removed in a future version. "
@@ -105,17 +128,21 @@ def after_evaluation(self, series_context: EvaluationSeriesContext, data: dict,
         if span is None:
             return data
 
-        attributes = {
+        attributes: Dict[str, AttributeValue] = {
             'feature_flag.context.id': series_context.context.fully_qualified_key,
             'feature_flag.key': series_context.key,
             'feature_flag.provider.name': 'LaunchDarkly',
         }
 
+        environment_id = self.__environment_id or _valid_environment_id(getattr(series_context, 'environment_id', None))
+        if environment_id is not None:
+            attributes['feature_flag.set.id'] = environment_id
+
         if detail.variation_index is not None:
-            attributes['feature_flag.result.variationIndex'] = str(detail.variation_index)
+            attributes['feature_flag.result.variationIndex'] = detail.variation_index
 
         if detail.reason.get('inExperiment'):
-            attributes['feature_flag.result.reason.inExperiment'] = 'true'
+            attributes['feature_flag.result.reason.inExperiment'] = True
 
         if self.__options.include_value or self.__options.include_variant:
             attributes['feature_flag.result.value'] = json.dumps(detail.value)