From 1dcf3bccabf1f041c7ab07f766590974099f54e2 Mon Sep 17 00:00:00 2001 From: Eoin Noble Date: Fri, 2 Oct 2020 00:08:00 +0100 Subject: [PATCH 1/7] Make event attributes immutable This PR makes it more difficult for someone to edit the attributes of an `Event` once it has been added to a `Span`, as well as adding some regression tests around the other properties of an `Event` to ensure they have similar protections. It isn't particularly straightforward or idiomatic to make objects truly immutable in Python, but if these steps don't go far enough let me know! Fixes #1038 --- .../src/opentelemetry/sdk/trace/__init__.py | 10 ++++--- .../src/opentelemetry/sdk/util/__init__.py | 7 +++++ opentelemetry-sdk/tests/trace/test_trace.py | 27 ++++++++++++++++++- 3 files changed, 40 insertions(+), 4 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index 0134ec7e775..7bdca0a0385 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -41,7 +41,7 @@ from opentelemetry.sdk import util from opentelemetry.sdk.resources import Resource from opentelemetry.sdk.trace import sampling -from opentelemetry.sdk.util import BoundedDict, BoundedList +from opentelemetry.sdk.util import BoundedDict, BoundedList, make_immutable_dict from opentelemetry.sdk.util.instrumentation import InstrumentationInfo from opentelemetry.trace import SpanContext from opentelemetry.trace.propagation import SPAN_KEY @@ -336,6 +336,10 @@ def _filter_attribute_values(attributes: types.Attributes): attributes.pop(attr_key) +def _make_event_attributes_immutable(event: EventBase) -> None: + event._attributes = make_immutable_dict(event.attributes) + + class Span(trace_api.Span): """See `opentelemetry.trace.Span`. @@ -399,6 +403,7 @@ def __init__( if events: for event in events: _filter_attribute_values(event.attributes) + _make_event_attributes_immutable(event) self.events.append(event) if links is None: @@ -557,8 +562,7 @@ def add_event( timestamp: Optional[int] = None, ) -> None: _filter_attribute_values(attributes) - if not attributes: - attributes = self._new_attributes() + attributes = make_immutable_dict(attributes) self._add_event( Event( name=name, diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/util/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/util/__init__.py index 09d7283cab7..e78f5324b63 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/util/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/util/__init__.py @@ -14,6 +14,7 @@ import datetime import threading from collections import OrderedDict, deque +from types import MappingProxyType try: # pylint: disable=ungrouped-imports @@ -45,6 +46,12 @@ def get_dict_as_key(labels): ) +def make_immutable_dict(attributes): + return MappingProxyType( + attributes.copy() if attributes else {} + ) + + class BoundedList(Sequence): """An append only list with a fixed max size. diff --git a/opentelemetry-sdk/tests/trace/test_trace.py b/opentelemetry-sdk/tests/trace/test_trace.py index 8c5544578d4..44c350847bf 100644 --- a/opentelemetry-sdk/tests/trace/test_trace.py +++ b/opentelemetry-sdk/tests/trace/test_trace.py @@ -561,7 +561,6 @@ def test_events(self): root.add_event("event0") # event name and attributes - now = time_ns() root.add_event( "event1", {"name": "pluto", "some_bools": [True, False]} ) @@ -599,6 +598,32 @@ def test_events(self): root.events[3].attributes, {"name": ("original_contents",)} ) + + def test_events_are_immutable(self): + event_properties = [ + prop for prop in dir(trace.EventBase) + if not prop.startswith("_") + ] + + with self.tracer.start_as_current_span("root") as root: + root.add_event("event0", {"name": ["birthday"]}) + event = root.events[0] + + for prop in event_properties: + with self.assertRaises(AttributeError): + setattr(event, prop, "something") + + def test_event_attributes_are_immutable(self): + with self.tracer.start_as_current_span("root") as root: + root.add_event("event0", {"name": ["birthday"]}) + event = root.events[0] + + with self.assertRaises(TypeError): + event.attributes["name"][0] = "happy" + + with self.assertRaises(TypeError): + event.attributes["name"] = "hello" + def test_invalid_event_attributes(self): self.assertEqual(trace_api.get_current_span(), trace_api.INVALID_SPAN) From 618013f414f3426feffedd5b10ea351e6cc8f538 Mon Sep 17 00:00:00 2001 From: Eoin Noble Date: Fri, 2 Oct 2020 00:21:18 +0100 Subject: [PATCH 2/7] Update CHANGELOG --- opentelemetry-sdk/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/opentelemetry-sdk/CHANGELOG.md b/opentelemetry-sdk/CHANGELOG.md index 71864282885..419988e6f2d 100644 --- a/opentelemetry-sdk/CHANGELOG.md +++ b/opentelemetry-sdk/CHANGELOG.md @@ -10,6 +10,8 @@ ([#1105](https://github.com/open-telemetry/opentelemetry-python/pull/1120)) - Allow for Custom Trace and Span IDs Generation - `IdsGenerator` for TracerProvider ([#1153](https://github.com/open-telemetry/opentelemetry-python/pull/1153)) +- Event attributes are now immutable + ([#1195](https://github.com/open-telemetry/opentelemetry-python/pull/1195)) ## Version 0.13b0 From dbb24224183a2512de0ac8c7ee9447851998ab09 Mon Sep 17 00:00:00 2001 From: Eoin Noble Date: Fri, 2 Oct 2020 10:43:58 +0100 Subject: [PATCH 3/7] Fix linting issues --- opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py | 6 +++++- opentelemetry-sdk/src/opentelemetry/sdk/util/__init__.py | 4 +--- opentelemetry-sdk/tests/trace/test_trace.py | 4 +--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index 7bdca0a0385..d6b141a40d8 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -41,7 +41,11 @@ from opentelemetry.sdk import util from opentelemetry.sdk.resources import Resource from opentelemetry.sdk.trace import sampling -from opentelemetry.sdk.util import BoundedDict, BoundedList, make_immutable_dict +from opentelemetry.sdk.util import ( + BoundedDict, + BoundedList, + make_immutable_dict +) from opentelemetry.sdk.util.instrumentation import InstrumentationInfo from opentelemetry.trace import SpanContext from opentelemetry.trace.propagation import SPAN_KEY diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/util/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/util/__init__.py index e78f5324b63..f8bba3c34ea 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/util/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/util/__init__.py @@ -47,9 +47,7 @@ def get_dict_as_key(labels): def make_immutable_dict(attributes): - return MappingProxyType( - attributes.copy() if attributes else {} - ) + return MappingProxyType(attributes.copy() if attributes else {}) class BoundedList(Sequence): diff --git a/opentelemetry-sdk/tests/trace/test_trace.py b/opentelemetry-sdk/tests/trace/test_trace.py index 44c350847bf..60db44a6a08 100644 --- a/opentelemetry-sdk/tests/trace/test_trace.py +++ b/opentelemetry-sdk/tests/trace/test_trace.py @@ -598,11 +598,9 @@ def test_events(self): root.events[3].attributes, {"name": ("original_contents",)} ) - def test_events_are_immutable(self): event_properties = [ - prop for prop in dir(trace.EventBase) - if not prop.startswith("_") + prop for prop in dir(trace.EventBase) if not prop.startswith("_") ] with self.tracer.start_as_current_span("root") as root: From 2f6e0a62818cbd59380dd94eeed395c7adf24edd Mon Sep 17 00:00:00 2001 From: Eoin Noble Date: Fri, 2 Oct 2020 10:46:51 +0100 Subject: [PATCH 4/7] Missing comma --- opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index d6b141a40d8..70476a36341 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -44,7 +44,7 @@ from opentelemetry.sdk.util import ( BoundedDict, BoundedList, - make_immutable_dict + make_immutable_dict, ) from opentelemetry.sdk.util.instrumentation import InstrumentationInfo from opentelemetry.trace import SpanContext From ed353adb355ecc4ce45db1ce6e1889e54884e85c Mon Sep 17 00:00:00 2001 From: Eoin Noble Date: Fri, 2 Oct 2020 10:56:47 +0100 Subject: [PATCH 5/7] Fix pylint warning --- opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py | 1 + 1 file changed, 1 insertion(+) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index 70476a36341..a0c29303024 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -340,6 +340,7 @@ def _filter_attribute_values(attributes: types.Attributes): attributes.pop(attr_key) +# pylint: disable=protected-access def _make_event_attributes_immutable(event: EventBase) -> None: event._attributes = make_immutable_dict(event.attributes) From 8ecf95980c5bb2c804807666fa4496d404ad1190 Mon Sep 17 00:00:00 2001 From: Eoin Noble Date: Sat, 3 Oct 2020 10:36:42 +0100 Subject: [PATCH 6/7] Keep all code in the trace module --- .../src/opentelemetry/sdk/trace/__init__.py | 17 ++++++----------- .../src/opentelemetry/sdk/util/__init__.py | 5 ----- 2 files changed, 6 insertions(+), 16 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index a0c29303024..19c3b8ceb2d 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -18,12 +18,11 @@ import concurrent.futures import json import logging -import random import threading import traceback from collections import OrderedDict from contextlib import contextmanager -from types import TracebackType +from types import MappingProxyType, TracebackType from typing import ( Any, Callable, @@ -41,11 +40,7 @@ from opentelemetry.sdk import util from opentelemetry.sdk.resources import Resource from opentelemetry.sdk.trace import sampling -from opentelemetry.sdk.util import ( - BoundedDict, - BoundedList, - make_immutable_dict, -) +from opentelemetry.sdk.util import BoundedDict, BoundedList from opentelemetry.sdk.util.instrumentation import InstrumentationInfo from opentelemetry.trace import SpanContext from opentelemetry.trace.propagation import SPAN_KEY @@ -340,9 +335,8 @@ def _filter_attribute_values(attributes: types.Attributes): attributes.pop(attr_key) -# pylint: disable=protected-access -def _make_event_attributes_immutable(event: EventBase) -> None: - event._attributes = make_immutable_dict(event.attributes) +def make_immutable_dict(attributes): + return MappingProxyType(attributes.copy() if attributes else {}) class Span(trace_api.Span): @@ -408,7 +402,8 @@ def __init__( if events: for event in events: _filter_attribute_values(event.attributes) - _make_event_attributes_immutable(event) + # pylint: disable=protected-access + event._attributes = make_immutable_dict(event.attributes) self.events.append(event) if links is None: diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/util/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/util/__init__.py index f8bba3c34ea..09d7283cab7 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/util/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/util/__init__.py @@ -14,7 +14,6 @@ import datetime import threading from collections import OrderedDict, deque -from types import MappingProxyType try: # pylint: disable=ungrouped-imports @@ -46,10 +45,6 @@ def get_dict_as_key(labels): ) -def make_immutable_dict(attributes): - return MappingProxyType(attributes.copy() if attributes else {}) - - class BoundedList(Sequence): """An append only list with a fixed max size. From 3812b35968e62af333ca15cda18505d19df748ed Mon Sep 17 00:00:00 2001 From: Eoin Noble Date: Mon, 5 Oct 2020 19:44:05 +0100 Subject: [PATCH 7/7] Rename helper function --- opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index 19c3b8ceb2d..43c22db0605 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -335,7 +335,7 @@ def _filter_attribute_values(attributes: types.Attributes): attributes.pop(attr_key) -def make_immutable_dict(attributes): +def _create_immutable_attributes(attributes): return MappingProxyType(attributes.copy() if attributes else {}) @@ -403,7 +403,9 @@ def __init__( for event in events: _filter_attribute_values(event.attributes) # pylint: disable=protected-access - event._attributes = make_immutable_dict(event.attributes) + event._attributes = _create_immutable_attributes( + event.attributes + ) self.events.append(event) if links is None: @@ -562,7 +564,7 @@ def add_event( timestamp: Optional[int] = None, ) -> None: _filter_attribute_values(attributes) - attributes = make_immutable_dict(attributes) + attributes = _create_immutable_attributes(attributes) self._add_event( Event( name=name,