Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

span: add is_recording_events #141

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions opentelemetry-api/src/opentelemetry/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,13 @@ def update_name(self, name: str) -> None:
on the implementation.
"""

def is_recording_events(self) -> bool:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should return a boolean value (most likely False) here too. I wonder why mypy didn't catch this. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmm, so what about

def get_context(self) -> "SpanContext":
?

Copy link
Member

@Oberon00 Oberon00 Sep 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems also wrong.
EDIT: I created #142.

"""Returns whether this span will be recorded.

Returns true if this Span is active and recording information like
events with the add_event operation and attributes using set_attribute.
"""


class TraceOptions(int):
"""A bitmask that represents options specific to the trace.
Expand Down
13 changes: 13 additions & 0 deletions opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,8 @@ def get_context(self):

def set_attribute(self, key: str, value: types.AttributeValue) -> None:
with self._lock:
if not self.is_recording_events():
return
has_ended = self.end_time is not None
if not has_ended:
if self.attributes is Span.empty_attributes:
Expand All @@ -302,6 +304,8 @@ def add_event(

def add_lazy_event(self, event: trace_api.Event) -> None:
with self._lock:
if not self.is_recording_events():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return
has_ended = self.end_time is not None
if not has_ended:
if self.events is Span.empty_events:
Expand All @@ -322,6 +326,8 @@ def add_link(

def add_lazy_link(self, link: "trace_api.Link") -> None:
with self._lock:
if not self.is_recording_events():
return
has_ended = self.end_time is not None
if not has_ended:
if self.links is Span.empty_links:
Expand All @@ -333,6 +339,8 @@ def add_lazy_link(self, link: "trace_api.Link") -> None:

def start(self):
with self._lock:
if not self.is_recording_events():
return
has_started = self.start_time is not None
if not has_started:
self.start_time = util.time_ns()
Expand All @@ -343,6 +351,8 @@ def start(self):

def end(self):
with self._lock:
if not self.is_recording_events():
return
if self.start_time is None:
raise RuntimeError("Calling end() on a not started span.")
has_ended = self.end_time is not None
Expand All @@ -362,6 +372,9 @@ def update_name(self, name: str) -> None:
return
self.name = name
Oberon00 marked this conversation as resolved.
Show resolved Hide resolved

def is_recording_events(self) -> bool:
return True


def generate_span_id():
"""Get a new random span ID.
Expand Down