Span and SpanContext implementation #58
Conversation
| from collections import Sequence | ||
|
|
||
|
|
||
| MAX_NUM_ATTRIBUTES = 32 |
There was a problem hiding this comment.
These might move to the exporters, see open-telemetry/opentelemetry-specification#182.
| return len(self._dq) | ||
|
|
||
| def __iter__(self): | ||
| return iter(self._dq) |
There was a problem hiding this comment.
Do we intend to handle RuntimeError: deque mutated during iteration or leave it as caller responsibility?
There was a problem hiding this comment.
How about making a defensive copy? 7836995
There was a problem hiding this comment.
Given most of the built-in collections throw exception when changes happened while iterating, it is fine to leave it. Unless we want this __iter__ to be bulletproof. I asked this question just to confirm this is what we intend.
BTW, what is 7836995?
There was a problem hiding this comment.
Catching this in the exporter (where we'd iterate through these lists) would be odd since the spans should be finished at that point, but I think it's better to make these bulletproof than risk the unhandled exception later.
| AttributeValue = typing.Union[str, bool, float] | ||
|
|
||
|
|
||
| class BoundedList(Sequence): |
There was a problem hiding this comment.
This class is likely to be used in metrics/logs as well, we might need to extract it to a common place. Probably not in this PR though.
| MAX_NUM_ATTRIBUTES, attributes) | ||
|
|
||
| if events is None: | ||
| self.events = BoundedList(MAX_NUM_EVENTS) |
There was a problem hiding this comment.
We don't have concrete measurement today, but I suspect it could be pretty expensive if most spans have no attributes/events/links.
Consider create a class variable Span.copy_on_write_bounded_list/dict and assign it as the default value. Allocate the real object when there are actual write operation.
There was a problem hiding this comment.
Changed in 87cac92, although it's probably worth profiling before making too many optimizations like this.
| import time | ||
|
|
||
| try: | ||
| time_ns = time.time_ns # noqa |
There was a problem hiding this comment.
Consider this time_ns = getattr(time, 'time_ns', lambda: int(time.time() * 1e9).
There was a problem hiding this comment.
Why do you like that better?
There was a problem hiding this comment.
Probably due to my background in C++ where people generally believe that exceptions are much heavier than conditional statement. I don't have strong preference here (as performance won't have a noticeable difference, it is mainly a matter of personal taste), up to you.
|
@Kami would be great to have your feedback here. Thanks. |
Breaking this off of the larger tracer SDK PR for discussion. See the OC span definition and java
Spanimplementations:DefaultSpanin the API andRecordEventsReadableSpanin the SDK.