span: add is_recording_events#141
Conversation
Add missing is_recording_events() function to span in API and SDK. The important point of this commit is to implement the check in functions like add_event, set_attribute and so son. Currently is_recording_events always returns true.
mauriciovasquezbernal
requested review from
Oberon00,
a-feld,
c24t,
carlosalberto,
lzchen,
reyang and
toumorokoshi
as code owners
| on the implementation. | ||
| """ | ||
|
|
||
| def is_recording_events(self) -> bool: |
There was a problem hiding this comment.
You should return a boolean value (most likely False) here too. I wonder why mypy didn't catch this. 🤔
There was a problem hiding this comment.
mmm, so what about
?| """Returns the flag whether this span will be recorded. | ||
|
|
||
| Returns true if this Span is active and recording information like | ||
| events with the AddEvent operation and attributes using SetAttributes. |
There was a problem hiding this comment.
These names are not Python-adjusted.
|
|
||
| def add_lazy_event(self, event: trace_api.Event) -> None: | ||
| with self._lock: | ||
| if not self.is_recording_events(): |
There was a problem hiding this comment.
As long as we always return True in is_recording_events I wonder if we need these checks at all yet. And if we want them, maybe they can be outside the lock (I think the sampling decision is made when creating the span already, so this should never change)?
There was a problem hiding this comment.
Well, I just wanted to put that logic in place regardless the value that is returned by is_recording_events (the same that otel-java does right now).
I am not sure if we can move those checks outside the lock, so I preferred to stay in the safe side and leave them inside.
Co-Authored-By: Christian Neumüller <christian+github@neumueller.me>
c24t
left a comment
c24t
left a comment
There was a problem hiding this comment.
The implementation looks good and according to spec, but I'm not sure about the utility of is_recording_events.
* chore: add c8 settings * fix: add .nycrc
4 participants
Add missing is_recording_events() function to span in API and SDK.
The important point of this commit is to implement the check in functions like
add_event, set_attribute and so son. Currently is_recording_events always
returns true.
Solves: #100