diff --git a/.changelog/5454.fixed b/.changelog/5454.fixed new file mode 100644 index 0000000000..cc73265525 --- /dev/null +++ b/.changelog/5454.fixed @@ -0,0 +1 @@ +`opentelemetry-configuration`: a declarative config key present with an empty (null) value on an object-typed node (e.g. `always_on:`, a `- service:` detector, or a metric `console:` exporter) is now treated the same as an explicit empty config (`always_on: {}`) instead of failing type dispatch or silently skipping the node. Both `dict`-typed nodes and dataclasses constructible with no arguments are covered. diff --git a/opentelemetry-configuration/src/opentelemetry/configuration/_conversion.py b/opentelemetry-configuration/src/opentelemetry/configuration/_conversion.py index c388b966b7..6eeaae4964 100644 --- a/opentelemetry-configuration/src/opentelemetry/configuration/_conversion.py +++ b/opentelemetry-configuration/src/opentelemetry/configuration/_conversion.py @@ -12,7 +12,7 @@ from __future__ import annotations from collections.abc import Mapping -from dataclasses import fields, is_dataclass +from dataclasses import MISSING, fields, is_dataclass from enum import Enum from types import UnionType from typing import Any, TypeVar, Union, get_args, get_origin, get_type_hints @@ -33,6 +33,24 @@ def _unwrap_optional(type_hint: Any) -> Any: return type_hint +def _is_empty_constructible_dataclass(unwrapped: Any) -> bool: + """True if ``unwrapped`` is a dataclass type instantiable with no args. + + A dataclass with only optional fields (all have a default or + default_factory) can be built as ``cls()``; one with a required field + cannot, and coercing a present null into it would raise ``TypeError``. + """ + return ( + isinstance(unwrapped, type) + and is_dataclass(unwrapped) + and all( + field.default is not MISSING + or field.default_factory is not MISSING + for field in fields(unwrapped) + ) + ) + + def _convert_value(value: Any, type_hint: Any) -> Any: """Convert a value according to its type hint. @@ -40,12 +58,25 @@ def _convert_value(value: Any, type_hint: Any) -> Any: dataclasses. Other values (primitives, enums, ``dict[str, Any]`` aliases) pass through unchanged. """ - if value is None: - return None - unwrapped = _unwrap_optional(type_hint) origin = get_origin(unwrapped) + if value is None: + # A mapping key present with an empty (null) YAML value parses to + # ``None``, which is otherwise indistinguishable from an absent key. + # For object-typed nodes the declarative-config spec treats a present + # null as "select this with an empty config" — e.g. ``always_on:`` is + # equivalent to ``always_on: {}`` and a metric ``console:`` exporter to + # ``console: {}``. Substitute an empty mapping and let the dict/ + # dataclass handling below build it, so downstream ``is not None`` type + # dispatch selects it. Scalar fields — and dataclasses with required + # fields, which cannot be defaulted — keep ``None``, so an absent + # optional section stays unset. + if origin is dict or _is_empty_constructible_dataclass(unwrapped): + value = {} + else: + return None + # list[X] — recurse on each element if origin is list and isinstance(value, list): args = get_args(unwrapped) diff --git a/opentelemetry-configuration/tests/file/test_loader.py b/opentelemetry-configuration/tests/file/test_loader.py index ea9689008b..2d047dcc14 100644 --- a/opentelemetry-configuration/tests/file/test_loader.py +++ b/opentelemetry-configuration/tests/file/test_loader.py @@ -282,11 +282,11 @@ class TestConfigLoaderEndToEnd(unittest.TestCase): trace_id_ratio_based: {ratio: 0.5} """ - def _load(self) -> OpenTelemetryConfiguration: + def _load(self, yaml: str | None = None) -> OpenTelemetryConfiguration: with tempfile.NamedTemporaryFile( mode="w", suffix=".yaml", delete=False ) as fh: - fh.write(self._YAML) + fh.write(self._YAML if yaml is None else yaml) path = fh.name try: return load_config_file(path) @@ -327,6 +327,35 @@ def test_typed_config_feeds_factory_function(self): self.assertIsInstance(processors[0], BatchSpanProcessor) self.assertIsInstance(processors[0].span_exporter, ConsoleSpanExporter) + def test_null_valued_required_field_node_survives_conversion(self): + # Regression test for #5451: ``jaeger_remote_development`` is nullable + # in the schema (so ``jaeger_remote_development:`` passes validation and + # is NOT rejected before conversion), yet its model has required + # fields. Coercing the present null into it would raise TypeError, so + # it must be left as None instead. The rest of the config still loads. + config = self._load( + """ +file_format: '1.0' +tracer_provider: + processors: + - batch: + exporter: + console: + sampler: + jaeger_remote_development: +""" + ) + + # Schema accepted the null, and conversion left the required-field + # node unset rather than crashing. + self.assertIsNone( + config.tracer_provider.sampler.jaeger_remote_development + ) + # A sibling nullable dict-typed node (console:) was still coerced. + self.assertEqual( + config.tracer_provider.processors[0].batch.exporter.console, {} + ) + class TestFileFormatValidation(unittest.TestCase): """Validate the file_format version per the configuration spec.""" diff --git a/opentelemetry-configuration/tests/test_conversion.py b/opentelemetry-configuration/tests/test_conversion.py index 6ae15c6e64..061df050df 100644 --- a/opentelemetry-configuration/tests/test_conversion.py +++ b/opentelemetry-configuration/tests/test_conversion.py @@ -11,6 +11,7 @@ from opentelemetry.configuration._common import _additional_properties from opentelemetry.configuration._conversion import _dict_to_dataclass from opentelemetry.configuration.models import ExemplarFilter +from opentelemetry.configuration.models import Sampler as SamplerConfig @dataclass @@ -42,6 +43,13 @@ class _WithEnum: filter: ExemplarFilter | None = None +@dataclass +class _WithMapping: + # ``dict[str, Any]``-typed node, like the sampler/detector leaf configs. + option: dict[str, Any] | None = None + name: str | None = None + + class TestDictToDataclass(unittest.TestCase): def test_raises_on_non_dataclass(self): # _dict_to_dataclass is internal and assumes cls is a dataclass. @@ -72,11 +80,25 @@ def test_converts_list_of_dataclasses(self): self.assertEqual(result.middle.items[0].value, 1) self.assertEqual(result.middle.items[1].value, 2) - def test_none_value_preserved(self): + def test_present_null_dataclass_coerced_to_empty_instance(self): + # A present-null value on a dataclass-typed node that can be built with + # no arguments (e.g. a metric ``console:`` exporter) becomes a + # defaulted instance, not None. Regression test for #5451. result = _dict_to_dataclass({"middle": None, "name": "test"}, _Outer) - self.assertIsNone(result.middle) + self.assertIsInstance(result.middle, _Middle) + self.assertIsNone(result.middle.inner) self.assertEqual(result.name, "test") + def test_present_null_dataclass_with_required_field_stays_none(self): + # ``jaeger_remote_development`` is nullable in the schema but its model + # (ExperimentalJaegerRemoteSampler) has required fields, so it cannot + # be defaulted. A present null must stay None rather than raising a + # TypeError trying to instantiate it. Regression test for #5451. + result = _dict_to_dataclass( + {"jaeger_remote_development": None}, SamplerConfig + ) + self.assertIsNone(result.jaeger_remote_development) + def test_missing_optional_fields_default_to_none(self): result = _dict_to_dataclass({}, _Outer) self.assertIsNone(result.middle) @@ -108,3 +130,23 @@ def test_enum_value_already_enum_passes_through(self): {"filter": ExemplarFilter.trace_based}, _WithEnum ) self.assertIs(result.filter, ExemplarFilter.trace_based) + + def test_present_null_mapping_coerced_to_empty_dict(self): + # A key present with a null YAML value (e.g. ``option:``) on a + # dict-typed node means "select with empty config", so it must become + # an empty mapping rather than None. Regression test for #5451. + result = _dict_to_dataclass({"option": None}, _WithMapping) + self.assertEqual(result.option, {}) + + def test_explicit_empty_mapping_preserved(self): + result = _dict_to_dataclass({"option": {}}, _WithMapping) + self.assertEqual(result.option, {}) + + def test_absent_mapping_stays_none(self): + # An omitted optional section must remain unset, unlike a present null. + result = _dict_to_dataclass({"name": "x"}, _WithMapping) + self.assertIsNone(result.option) + + def test_present_null_scalar_stays_none(self): + result = _dict_to_dataclass({"name": None}, _WithMapping) + self.assertIsNone(result.name) diff --git a/opentelemetry-configuration/tests/test_meter_provider.py b/opentelemetry-configuration/tests/test_meter_provider.py index 5e1d7e686a..691df23637 100644 --- a/opentelemetry-configuration/tests/test_meter_provider.py +++ b/opentelemetry-configuration/tests/test_meter_provider.py @@ -9,6 +9,7 @@ import unittest from unittest.mock import MagicMock, patch +from opentelemetry.configuration._conversion import _dict_to_dataclass from opentelemetry.configuration._meter_provider import ( configure_meter_provider, create_meter_provider, @@ -186,6 +187,23 @@ def test_console_exporter(self): self.assertIsInstance(reader, PeriodicExportingMetricReader) self.assertIsInstance(reader._exporter, ConsoleMetricExporter) + def test_null_valued_console_exporter_from_parsed_config(self): + # Regression test for #5451: a dataclass-typed exporter written as + # ``console:`` (present, null) must behave like ``console: {}`` rather + # than requiring an explicit empty mapping. + config = _dict_to_dataclass( + { + "readers": [ + {"periodic": {"exporter": {"console": None}}}, + ] + }, + MeterProviderConfig, + ) + provider = create_meter_provider(config) + reader = provider._metric_readers[0] + self.assertIsInstance(reader, PeriodicExportingMetricReader) + self.assertIsInstance(reader._exporter, ConsoleMetricExporter) + def test_periodic_reader_default_interval(self): config = self._make_periodic_config( PushMetricExporterConfig(console=ConsoleMetricExporterConfig()) diff --git a/opentelemetry-configuration/tests/test_resource.py b/opentelemetry-configuration/tests/test_resource.py index b822a37f8f..c2147df09c 100644 --- a/opentelemetry-configuration/tests/test_resource.py +++ b/opentelemetry-configuration/tests/test_resource.py @@ -7,6 +7,7 @@ import unittest from unittest.mock import MagicMock, patch +from opentelemetry.configuration._conversion import _dict_to_dataclass from opentelemetry.configuration._exceptions import ConfigurationError from opentelemetry.configuration._resource import create_resource from opentelemetry.configuration.models import ( @@ -376,6 +377,17 @@ def test_explicit_service_name_overrides_env_var(self): resource = create_resource(config) self.assertEqual(resource.attributes[SERVICE_NAME], "explicit-svc") + def test_null_valued_detector_from_parsed_config(self): + # Regression test for #5451: a detector written as ``- service:`` + # (present, null) is parsed to ``{"service": None}``. It must run the + # detector just like ``- service: {}`` rather than being skipped. + config = _dict_to_dataclass( + {"detection_development": {"detectors": [{"service": None}]}}, + ResourceConfig, + ) + resource = create_resource(config) + self.assertIn(SERVICE_INSTANCE_ID, resource.attributes) + def test_service_detector_not_run_when_absent(self): resource = create_resource(ResourceConfig()) self.assertNotIn(SERVICE_INSTANCE_ID, resource.attributes) diff --git a/opentelemetry-configuration/tests/test_tracer_provider.py b/opentelemetry-configuration/tests/test_tracer_provider.py index b8fe78f82a..492f0a83d0 100644 --- a/opentelemetry-configuration/tests/test_tracer_provider.py +++ b/opentelemetry-configuration/tests/test_tracer_provider.py @@ -10,6 +10,7 @@ from unittest.mock import MagicMock, patch from opentelemetry import trace as trace_api +from opentelemetry.configuration._conversion import _dict_to_dataclass from opentelemetry.configuration._tracer_provider import ( configure_tracer_provider, create_tracer_provider, @@ -249,6 +250,39 @@ def test_no_sampler_raises_configuration_error(self): with self.assertRaises(ConfigurationError): self._make_provider(SamplerConfig()) + def test_null_valued_always_on_from_parsed_config(self): + # Regression test for #5451: a leaf sampler written as ``always_on:`` + # (present, null) is parsed to ``None`` by the YAML loader. It must be + # treated the same as ``always_on: {}`` rather than failing type + # dispatch. + sampler_config = _dict_to_dataclass({"always_on": None}, SamplerConfig) + provider = self._make_provider(sampler_config) + self.assertIs(provider.sampler, ALWAYS_ON) + + def test_null_valued_parent_based_delegates_from_parsed_config(self): + # Regression test for #5451: the exact parent_based config from the + # issue, where every leaf delegate is written with a null value. + sampler_config = _dict_to_dataclass( + { + "parent_based": { + "root": {"always_on": None}, + "remote_parent_sampled": {"always_on": None}, + "remote_parent_not_sampled": {"always_off": None}, + "local_parent_sampled": {"always_on": None}, + "local_parent_not_sampled": {"always_off": None}, + } + }, + SamplerConfig, + ) + provider = self._make_provider(sampler_config) + sampler = provider.sampler + self.assertIsInstance(sampler, ParentBased) + self.assertIs(sampler._root, ALWAYS_ON) + self.assertIs(sampler._remote_parent_sampled, ALWAYS_ON) + self.assertIs(sampler._remote_parent_not_sampled, ALWAYS_OFF) + self.assertIs(sampler._local_parent_sampled, ALWAYS_ON) + self.assertIs(sampler._local_parent_not_sampled, ALWAYS_OFF) + def test_user_defined_sampler_loaded_via_entry_point(self): mock_sampler = MagicMock(spec=Sampler) mock_class = MagicMock(return_value=mock_sampler)