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

Add setStatus in Span #213

Merged
merged 37 commits into from
Oct 16, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
b837c9f
Span add override parameters
hectorhdzg Sep 26, 2019
a12fe5d
Make lint happy
hectorhdzg Sep 26, 2019
276ecb4
Addressing comments
hectorhdzg Sep 27, 2019
2b90351
Addressing comments
hectorhdzg Sep 27, 2019
eccef1a
Allowing 0 as start and end time
hectorhdzg Sep 28, 2019
a187fec
Fix lint issues
hectorhdzg Sep 28, 2019
a43c980
Merge remote-tracking branch 'upstream/master'
hectorhdzg Oct 2, 2019
8dfb44e
Merge remote-tracking branch 'upstream/master'
hectorhdzg Oct 8, 2019
77d3649
Add code coverage
hectorhdzg Oct 8, 2019
f3af20f
Revert latest commit
hectorhdzg Oct 9, 2019
1229bc7
Merge remote-tracking branch 'upstream/master'
hectorhdzg Oct 9, 2019
e54a053
Adding setStatus in Span
hectorhdzg Oct 9, 2019
174ac90
Fixing lint issues
hectorhdzg Oct 9, 2019
2015944
Addressing comments
hectorhdzg Oct 10, 2019
437b5a5
Fixing mypy issues
hectorhdzg Oct 10, 2019
8ae0532
Fixing old python ver issue
hectorhdzg Oct 10, 2019
798551b
Fixing formatting issue
hectorhdzg Oct 10, 2019
35a93d4
Fixing issue with trailing whitespace
hectorhdzg Oct 10, 2019
db45017
Addressing comments
hectorhdzg Oct 11, 2019
7fe57a1
Fixing lint issues
hectorhdzg Oct 11, 2019
57ebc24
Fixing formatting issues
hectorhdzg Oct 11, 2019
27df21b
Fixing lint issues
hectorhdzg Oct 11, 2019
4ccc142
Fixing issues generating docs
hectorhdzg Oct 12, 2019
d08c3db
Addressing comments
hectorhdzg Oct 15, 2019
5c39740
Merge branch 'master' into spanStatus
hectorhdzg Oct 15, 2019
7265b92
Fixing lint issue after merge
hectorhdzg Oct 15, 2019
98960ef
Lint fix
hectorhdzg Oct 15, 2019
a714d20
Fix tests
hectorhdzg Oct 15, 2019
ac7cf0d
Enum docstring fixes
c24t Oct 15, 2019
1863412
Docstring tweaks
c24t Oct 15, 2019
27ba6a4
Fix wrap
c24t Oct 15, 2019
5e17bb7
Merge pull request #1 from c24t/spanStatus-docs-fixes
hectorhdzg Oct 15, 2019
d8f8c26
Addressing comments
hectorhdzg Oct 15, 2019
673b5be
Merge
hectorhdzg Oct 15, 2019
c5e71eb
Addressing comments
hectorhdzg Oct 16, 2019
ce3552b
Fix mangled comment wrapping
c24t Oct 16, 2019
04709bd
Wrap some text
c24t Oct 16, 2019
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
12 changes: 11 additions & 1 deletion docs/opentelemetry.trace.rst
Original file line number Diff line number Diff line change
@@ -1,4 +1,14 @@
opentelemetry.trace package
===========================

.. automodule:: opentelemetry.trace
Submodules
----------

.. toctree::

opentelemetry.trace.status

Module contents
---------------

.. automodule:: opentelemetry.trace
7 changes: 7 additions & 0 deletions docs/opentelemetry.trace.status.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
opentelemetry.trace.status
==========================

.. automodule:: opentelemetry.trace.status
:members:
:undoc-members:
:show-inheritance:
6 changes: 6 additions & 0 deletions opentelemetry-api/src/opentelemetry/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@
import typing
from contextlib import contextmanager

from opentelemetry.trace.status import Status
from opentelemetry.util import loader, types

# TODO: quarantine
Expand Down Expand Up @@ -227,6 +228,11 @@ def is_recording_events(self) -> bool:
events with the add_event operation and attributes using set_attribute.
"""

def set_status(self, status: Status) -> None:
"""Sets the Status of the Span. If used, this will override the default
Span status, which is OK.
"""

def __enter__(self) -> "Span":
"""Invoked when `Span` is used as a context manager.

Expand Down
185 changes: 185 additions & 0 deletions opentelemetry-api/src/opentelemetry/trace/status.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
# Copyright 2019, OpenTelemetry Authors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

import enum
import typing


class StatusCanonicalCode(enum.Enum):
"""Represents the canonical set of status codes of a finished Span."""

OK = 0
"""Not an error, returned on success."""
Copy link
Member

@Oberon00 Oberon00 Oct 16, 2019

Choose a reason for hiding this comment

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

AFAIK, attribute docstrings are not a thing in Python. So I guess to the interpreter these are just string expressions without any effect. Thus, and for consistency, I'd prefer using #: comments like elsewhere (

#: Default value. Indicates that the span is used internally in the application.
INTERNAL = 0
#: Indicates that the span describes an operation that handles a remote request.
SERVER = 1
#: Indicates that the span describes a request to some remote service.
CLIENT = 2
#: Indicates that the span describes a producer sending a message to a
#: broker. Unlike client and server, there is usually no direct critical
#: path latency relationship between producer and consumer spans.
PRODUCER = 3
#: Indicates that the span describes consumer receiving a message from a
#: broker. Unlike client and server, there is usually no direct critical
#: path latency relationship between producer and consumer spans.
CONSUMER = 4
) to these "docstrings".

Copy link
Member

Choose a reason for hiding this comment

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

To the interpreter, but not to sphinx! Sphinx handles both "docstring"-style strings like this and regular comments.

I don't have a strong preference here, either comment style works for me as long as the comments and members line up in the generated docs.

@Oberon00 what's with the : in #:?


CANCELLED = 1
"""The operation was cancelled, typically by the caller."""

UNKNOWN = 2
"""Unknown error.

For example, this error may be returned when a Status value received from
another address space belongs to an error space that is not known in this
address space. Also errors raised by APIs that do not return enough error
information may be converted to this error.
"""

INVALID_ARGUMENT = 3
"""The client specified an invalid argument.

Note that this differs from FAILED_PRECONDITION. INVALID_ARGUMENT indicates
arguments that are problematic regardless of the state of the system (e.g.,
a malformed file name).
"""

DEADLINE_EXCEEDED = 4
"""The deadline expired before the operation could complete.

For operations that change the state of the system, this error may be
returned even if the operation has completed successfully. For example, a
successful response from a server could have been delayed long
"""

NOT_FOUND = 5
"""Some requested entity (e.g., file or directory) was not found.

Note to server developers: if a request is denied for an entire class of
users, such as gradual feature rollout or undocumented whitelist, NOT_FOUND
may be used. If a request is denied for some users within a class of users,
such as user-based access control, PERMISSION_DENIED must be used.
"""

ALREADY_EXISTS = 6
"""The entity that a client attempted to create (e.g., file or directory)
already exists.
"""

PERMISSION_DENIED = 7
"""The caller does not have permission to execute the specified operation.

PERMISSION_DENIED must not be used for rejections caused by exhausting some
resource (use RESOURCE_EXHAUSTED instead for those errors).
PERMISSION_DENIED must not be used if the caller can not be identified (use
UNAUTHENTICATED instead for those errors). This error code does not imply
the request is valid or the requested entity exists or satisfies other
pre-conditions.
"""

RESOURCE_EXHAUSTED = 8
"""Some resource has been exhausted, perhaps a per-user quota, or perhaps
the entire file system is out of space.
"""

FAILED_PRECONDITION = 9
"""The operation was rejected because the system is not in a state required
for the operation's execution.

For example, the directory to be deleted is non-empty, an rmdir operation
is applied to a non-directory, etc. Service implementors can use the
following guidelines to decide between FAILED_PRECONDITION, ABORTED, and
UNAVAILABLE:

(a) Use UNAVAILABLE if the client can retry just the failing call.
(b) Use ABORTED if the client should retry at a higher level (e.g.,
when a client-specified test-and-set fails, indicating the client
should restart a read-modify-write sequence).
(c) Use FAILED_PRECONDITION if the client should not retry until the
system state has been explicitly fixed.

E.g., if an "rmdir" fails because the directory is non-empty,
FAILED_PRECONDITION should be returned since the client should not retry
unless the files are deleted from the directory.
"""

ABORTED = 10
"""The operation was aborted, typically due to a concurrency issue such as a
sequencer check failure or transaction abort.

See the guidelines above for deciding between FAILED_PRECONDITION, ABORTED,
and UNAVAILABLE.
"""

OUT_OF_RANGE = 11
"""The operation was attempted past the valid range.

E.g., seeking or reading past end-of-file. Unlike INVALID_ARGUMENT, this
error indicates a problem that may be fixed if the system state changes.
For example, a 32-bit file system will generate INVALID_ARGUMENT if asked
to read at an offset that is not in the range [0,2^32-1],but it will
generate OUT_OF_RANGE if asked to read from an offset past the current file
size. There is a fair bit of overlap between FAILED_PRECONDITION and
OUT_OF_RANGE. We recommend using OUT_OF_RANGE (the more specific error)
when it applies so that callers who are iterating through a space can
easily look for an OUT_OF_RANGE error to detect when they are done.
"""

UNIMPLEMENTED = 12
"""The operation is not implemented or is not supported/enabled in this
service.
"""

INTERNAL = 13
"""Internal errors.

This means that some invariants expected by the underlying system have been
broken. This error code is reserved for serious errors.
"""

UNAVAILABLE = 14
"""The service is currently unavailable.

This is most likely a transient condition, which can be corrected by
retrying with a backoff. Note that it is not always safe to retry
non-idempotent operations.
"""

DATA_LOSS = 15
"""Unrecoverable data loss or corruption."""

UNAUTHENTICATED = 16
"""The request does not have valid authentication credentials for the
operation.
"""


class Status:
"""Represents the status of a finished Span.

Args:
canonical_code: The canonical status code that describes the result
status of the operation.
description: An optional description of the status.
"""

def __init__(
self,
canonical_code: "StatusCanonicalCode" = StatusCanonicalCode.OK,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
canonical_code: "StatusCanonicalCode" = StatusCanonicalCode.OK,
canonical_code: StatusCanonicalCode,
  • No quotes for types if not required.
  • I'd remove the default argument for the code: If you explicitly construct a status, most of the time it won't be OK.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the quotes for consistency with other files, I thought it was some kind of coding standard used here, if this is an issue maybe need to be addressed everywhere in the code.

Related to default status, spec says default is OK, that is the reason I added the default parameter
https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-tracing.md#set-status

Copy link
Member

Choose a reason for hiding this comment

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

I added the quotes for consistency with other files

Very much a nitpick, but I think it actually is a good idea to quote all non-standard types in these annotations. This way we can move classes around within modules without mypy complaining about forward references. One less mypy annoyance to deal with this way.

description: typing.Optional[str] = None,
):
self._canonical_code = canonical_code
self._description = description

@property
Copy link
Member

Choose a reason for hiding this comment

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

Why make these properties instead of just exposing the attributes?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Oberon00 commented before related this, using properties would be more pythonic, I'm fine with any approach

#213 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

I agree they shouldn't be getters too, I just don't see the point of exposing existing attrs under a different name.

I think it'd be fine to just expose code and desc, but if you want to hide them behind attributes I think you should change the underlying attrs to _code and _desc (or more conventionally: _canonical_code, _description).

Copy link
Member

Choose a reason for hiding this comment

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

@hectorhdzg: Could you please comment on why you made them properties instead of simply public attributes?

Copy link
Member Author

Choose a reason for hiding this comment

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

is_ok require a special getter because is doing an operation to get the value, I guess other two could be just simple attributes. I felt is just cleaner to have them as properties as well, I can update if there are strong opinions about this

Copy link
Member

Choose a reason for hiding this comment

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

No strong opinion as far as I'm concerned, it's just surprising to see properties wrap public attributes like this. We have both properties and bare attributes in the API now, and don't really have consistent reasons for using one over the other.

def canonical_code(self) -> "StatusCanonicalCode":
hectorhdzg marked this conversation as resolved.
Show resolved Hide resolved
"""Represents the canonical status code of a finished Span."""
return self._canonical_code

@property
def description(self) -> typing.Optional[str]:
"""Status description"""
return self._description

@property
def is_ok(self) -> bool:
"""Returns false if this represents an error, true otherwise."""
return self._canonical_code is StatusCanonicalCode.OK
9 changes: 9 additions & 0 deletions opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ def __init__(
self.kind = kind

self.span_processor = span_processor
self.status = trace_api.Status()
Copy link
Member

@Oberon00 Oberon00 Oct 16, 2019

Choose a reason for hiding this comment

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

We could add a constant of type Status named OK for the Status(OK, None) object in the status module and use it here to save object allocations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why adding a constant if this is already the default Status?, is this some kind of pattern? sound a little odd for me

Copy link
Member

Choose a reason for hiding this comment

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

It's a small optimization to avoid creating a new "OK" status with each new span when they could all use the same instance instead.

OK statuses are all alike, every not-OK status is not-OK in its own way. So we may as well use a single instance for the OK status.

self._lock = threading.Lock()

if attributes is None:
Expand Down Expand Up @@ -285,6 +286,14 @@ def update_name(self, name: str) -> None:
def is_recording_events(self) -> bool:
return True

def set_status(self, status: trace_api.Status) -> None:
with self._lock:
has_ended = self.end_time is not None
if has_ended:
logger.warning("Calling set_status() on an ended span.")
return
self.status = status
hectorhdzg marked this conversation as resolved.
Show resolved Hide resolved


def generate_span_id() -> int:
"""Get a new random span ID.
Expand Down
31 changes: 31 additions & 0 deletions opentelemetry-sdk/tests/trace/test_trace.py
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,24 @@ def test_start_span(self):
span.start()
self.assertEqual(start_time, span.start_time)

# default status
self.assertTrue(span.status.is_ok)
self.assertIs(
span.status.canonical_code, trace_api.status.StatusCanonicalCode.OK
)
self.assertIs(span.status.description, None)

# status
new_status = trace_api.status.Status(
trace_api.status.StatusCanonicalCode.CANCELLED, "Test description"
)
span.set_status(new_status)
self.assertIs(
span.status.canonical_code,
trace_api.status.StatusCanonicalCode.CANCELLED,
)
self.assertIs(span.status.description, "Test description")

def test_span_override_start_and_end_time(self):
"""Span sending custom start_time and end_time values"""
span = trace.Span("name", mock.Mock(spec=trace_api.SpanContext))
Expand Down Expand Up @@ -334,6 +352,19 @@ def test_ended_span(self):
root.update_name("xxx")
self.assertEqual(root.name, "root")

new_status = trace_api.status.Status(
trace_api.status.StatusCanonicalCode.CANCELLED,
"Test description",
)
root.set_status(new_status)
# default status
self.assertTrue(root.status.is_ok)
self.assertEqual(
root.status.canonical_code,
trace_api.status.StatusCanonicalCode.OK,
)
self.assertIs(root.status.description, None)


def span_event_start_fmt(span_processor_name, span_name):
return span_processor_name + ":" + span_name + ":start"
Expand Down