Events should be immutable and should preserve the order in which they are set

[lzchen]

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#add-events

[eoinnoble]

Hi there! I was interested in trying my hand at this but had a couple of questions first.

When the specification says

The Event SHOULD be an immutable type.

obviously that means e._name = 'new name' should fail, but does it also mean that e._attributes[0]['something'] = 'edited' should fail?

And when the specification says

Events SHOULD preserve the order in which they are recorded. This will typically match the ordering of the events' timestamps, but events may be recorded out-of-order using custom timestamps.

I read that as meaning that an event could appear in the list of events after another event that had a later timestamp, like this:

s._add_event(Event('a', timestamp=1))
s._add_event(Event('b', timestamp=3))
s._add_event(Event('c', timestamp=2))
list(s.events)
>>> [Event('a', timestamp=1), Event('b', timestamp=3), Event('c', timestamp=2)]

Is that correct or would we expect Event('c', timestamp=2) to appear before Event('b', timestamp=3) in events? I took a quick look at the JS libraries and couldn't see them doing that anywhere but wanted to double-check.

[lzchen]

obviously that means e._name = 'new name' should fail, but does it also mean that e._attributes[0]['something'] = 'edited' should fail?

I believe so.

Is that correct or would we expect Event('c', timestamp=2) to appear before Event('b', timestamp=3) in events?

I believe the former.

[eoinnoble]

Ok, maybe recursively transforming _attributes into nested tuples will be necessary in that case? I can try that out but it seems less idiomatic than overriding __setattr__ and __delattr__.

If events is already ordered as required by the specification perhaps this issue can be renamed to be solely about event immutability?

[lzchen]

You can do something similar to this or simply use nested tuples. I don't think you can override __setattr__ because then you won't be able to set anything in the __init__ method.

I believe your second question is correct, only about immutability if order is already preserved.

[eoinnoble]

I just want to make sure we want to go this far into the internals. As the code stands right now something like this will fail:

with self.tracer.start_as_current_span("root") as root:
    root.add_event("event0")

root.events[0].name = "event1"
>>> AttributeError: can't set attribute

This is because we already wrap most of the properties of Event and EventBase in @property decorators, so any attempt to directly change attributes, name or timestamp will fail. I feel like for those top-level properties we don't need to prevent someone from tinkering with _attributes, _name or _timestamp? This comment from the PR you linked to puts it well:

I don't think we need to make it literally impossible to modify the object. Given this is Python, I think anyone included to do it will find a way. We should just make sure we inform the person that they shouldn't do it. If they want to go out of their way and still modify, I think we should probably allow that.

What I do think still needs to be addressed is something like this:

with self.tracer.start_as_current_span("root") as root:
    root.add_event("event0", {"name": ["birthday"]})

root.events[0].attributes["name"][0] = "hello"  # Fails because `root.events[0].attributes["name"]` is a tuple
root.events[0].attributes["name"] = "hello"     # Succeeds because `root.events[0].attributes` is a dict

You can assign this issue to me and I can work on that specific example of editing the contents of attributes if you agree with the change in focus.

[lzchen]

I think we would still want to prevent users from modifying the dict elements like in root.events[0].attributes["name"] = "hello" .
I believe you can do this by utilizing a MappingProxyType similar to what we do in baggage.