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 23 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
20 changes: 20 additions & 0 deletions opentelemetry-api/src/opentelemetry/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,23 @@
import typing
from contextlib import contextmanager

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

__all__ = [
Copy link
Member

Choose a reason for hiding this comment

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

nit: sort alphabetically

Copy link
Member

Choose a reason for hiding this comment

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

This is all so we can keep Status and StatusCanonicalCode in the opentelemetry.trace namespace right? If that's the intention then it might not make sense to move them out of this module after all. But why not just use opentelemetry.trace.status?

Also AFAICT __all__ doesn't have any effect here. Since these classes are already in the opentelemetry.trace namespace, you'd import them all anyway with the default wildcard import behavior.

Copy link
Member

Choose a reason for hiding this comment

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

@reyang, @toumorokoshi, this question is for you.

As I understand it, we only explicitly import submodules in package init files so users don't get AttributeErrors when they do something like this:

import opentelemetry.sdk
opentelemetry.sdk.trace

But in that case we don't need __all__ to include the submodule names, we just need to import them in the package init file.

It's actually not clear to me why we'd ever use __all__ except to prevent modules or classes from being wildcard-imported. Which isn't how we're using it now in e.g. .ext.azure_monitor and .context.propagation.

What does __all__ give us here?

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 removed all, was having issues with docs creation for this, talked to @lzchen and realized this was not needed, so now opentelemetry.trace.status would be used to access Status and StatusCanonicalCode

Copy link
Member

@c24t c24t Oct 15, 2019

Choose a reason for hiding this comment

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

I agree that it's not needed here since we don't care about hiding the status module, but I see now that "preventing imports" is exactly what's happening in the azure exporter case. That is, it's intentional that this:

from opentelemetry.ext.azure_monitor import *

doesn't dump e.g. protocol into the current scope (but does incidentally import it).

So we still have an open question: how much work do we want to put into supporting wildcard imports? I think these __all__ declarations are going to get out of sync with the project package structure unless we start testing them somehow.

And as an aside: even if we do a bunch of namespace munging in package init files, we're still going to end up exposing the package structure to users. One weird quirk of the import system is that we can't hide internal import paths. E.g. after importing AzureMonitorSpanExporter the module opentelemetry.ext.azure_monitor.protocol is available. Selectively exposing packages/modules/classes in package init files may be a losing battle.

Copy link
Member

Choose a reason for hiding this comment

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

The only good reason I can tell for __all__ is because people can do from xyz import * (for convenience or laziness) and selectively export stuff as intended.
Given we don't allow from xyz import *, I don't see a good reason to use __all__ (if the purpose is to hide some private stuff, we should just use _ prefix).

"Status",
"StatusCanonicalCode",
"Link",
"Event",
"SpanKind",
"Span",
"SpanContext",
"TraceOptions",
"TraceState",
"DefaultSpan",
"Tracer",
]

# TODO: quarantine
ParentSpan = typing.Optional[typing.Union["Span", "SpanContext"]]

Expand Down Expand Up @@ -226,6 +241,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.
"""


class TraceOptions(int):
"""A bitmask that represents options specific to the trace.
Expand Down
168 changes: 168 additions & 0 deletions opentelemetry-api/src/opentelemetry/trace/status.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
# 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."""

# pylint: disable=pointless-string-statement
"""Not an error, returned on success.
"""
Copy link
Member

Choose a reason for hiding this comment

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

These are off by one in the generated docs since they're parsed as attribute docstrings. Since I had to make the change to check it anyway I put it in a PR for this branch: hectorhdzg#1.

OK = 0

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

"""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.
"""
UNKNOWN = 2

"""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).
"""
INVALID_ARGUMENT = 3

"""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
"""
DEADLINE_EXCEEDED = 4

"""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.
"""
NOT_FOUND = 5

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

"""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.
"""
PERMISSION_DENIED = 7

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

"""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.
"""
FAILED_PRECONDITION = 9

"""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."""
ABORTED = 10

"""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.
"""
OUT_OF_RANGE = 11

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

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

"""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.
"""
UNAVAILABLE = 14

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

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


class Status:
"""Represents the status of a finished Span
hectorhdzg marked this conversation as resolved.
Show resolved Hide resolved

Args:
canonical_code: Represents the canonical set of status codes of a
finished Span
hectorhdzg marked this conversation as resolved.
Show resolved Hide resolved
description: Description of the Status.
hectorhdzg marked this conversation as resolved.
Show resolved Hide resolved
"""

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.code = canonical_code
self.desc = 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
"""StatusCanonicalCode represents the canonical set of status codes
of a finished Span.
"""
return self.code

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

@property
def is_ok(self) -> bool:
"""Returns false if this Status represents an error, else
returns true"""
return self.canonical_code == StatusCanonicalCode.OK
4 changes: 4 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,9 @@ def update_name(self, name: str) -> None:
def is_recording_events(self) -> bool:
return True

def set_status(self, status: trace_api.Status) -> None:
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
14 changes: 14 additions & 0 deletions opentelemetry-sdk/tests/trace/test_trace.py
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,20 @@ def test_start_span(self):
span.start()
self.assertEqual(start_time, span.start_time)

# default status
self.assertTrue(span.status.is_ok)
self.assertEqual(
hectorhdzg marked this conversation as resolved.
Show resolved Hide resolved
span.status.canonical_code, trace_api.StatusCanonicalCode.OK
)
self.assertIs(span.status.description, None)

# status
new_status = trace_api.Status(
trace_api.StatusCanonicalCode.CANCELLED, "Test description"
)
span.set_status(new_status)
self.assertEqual(span.status, new_status)
hectorhdzg marked this conversation as resolved.
Show resolved Hide resolved

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