Skip to content

Commit

Permalink
Make event attributes immutable
Browse files Browse the repository at this point in the history
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
  • Loading branch information
eoinnoble committed Oct 1, 2020
1 parent c8df54b commit 1dcf3bc
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 4 deletions.
10 changes: 7 additions & 3 deletions opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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`.
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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,
Expand Down
7 changes: 7 additions & 0 deletions opentelemetry-sdk/src/opentelemetry/sdk/util/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import datetime
import threading
from collections import OrderedDict, deque
from types import MappingProxyType

try:
# pylint: disable=ungrouped-imports
Expand Down Expand Up @@ -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.
Expand Down
27 changes: 26 additions & 1 deletion opentelemetry-sdk/tests/trace/test_trace.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]}
)
Expand Down Expand Up @@ -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)

Expand Down

0 comments on commit 1dcf3bc

Please sign in to comment.